Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Re-merge "Add chunk offset to file id key to make each chunk have a unique key" from pull 825 #865

Merged
merged 4 commits into from
Oct 31, 2024
Merged

Re-merge "Add chunk offset to file id key to make each chunk have a unique key" from pull 825 #865

merged 4 commits into from
Oct 31, 2024

Conversation

sfc-gh-psaha
Copy link
Contributor

@sfc-gh-psaha sfc-gh-psaha commented Oct 18, 2024 •
edited by sfc-gh-xhuang
Loading

Add chunk offset to file id key to make each chunk have a unique key from #825

…ve a uni… (#848)"

This reverts commit 0930648.

@sfc-gh-psaha sfc-gh-psaha requested review from sfc-gh-tzhang and a team as code owners October 18, 2024 21:03
@sfc-gh-psaha sfc-gh-psaha force-pushed the psaha-unrevert-interleaved branch from 165b395 to 3a2ab2b Compare October 18, 2024 21:04
sfc-gh-tzhang
sfc-gh-tzhang requested changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-tzhang sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hold-off the merge until the server side change is in prod, do you know which release it will in?

@sfc-gh-psaha
Copy link
Contributor Author

Let's hold-off the merge until the server side change is in prod, do you know which release it will in?

It will be in 8.41. What's the risk if we merge now? False alarms in test environments?

@sfc-gh-psaha sfc-gh-psaha force-pushed the psaha-unrevert-interleaved branch from 3a2ab2b to 6707e0d Compare October 30, 2024 17:42
@sfc-gh-psaha
Copy link
Contributor Author

Let's hold-off the merge until the server side change is in prod, do you know which release it will in?

It will be in 8.41. What's the risk if we merge now? False alarms in test environments?

The plan is we'd merge this and enable server side detection fix starting from this version. PTAL.

sfc-gh-tzhang
sfc-gh-tzhang approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-tzhang sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure the server side change is enabled before this change is released, thanks!

@sfc-gh-xhuang sfc-gh-xhuang changed the title Revert "Revert "Add chunk offset to file id key to make each chunk ha… Re-merge "Add chunk offset to file id key to make each chunk ha… Oct 31, 2024
@sfc-gh-xhuang sfc-gh-xhuang changed the title Re-merge "Add chunk offset to file id key to make each chunk ha… Re-merge "Add chunk offset to file id key to make each chunk have a unique key" from pull 825 Oct 31, 2024
@sfc-gh-psaha sfc-gh-psaha force-pushed the psaha-unrevert-interleaved branch 2 times, most recently from f1262aa to e1f2073 Compare October 31, 2024 17:29
@sfc-gh-psaha sfc-gh-psaha force-pushed the psaha-unrevert-interleaved branch from e1f2073 to 17de48f Compare October 31, 2024 17:31
sfc-gh-hmadan
sfc-gh-hmadan reviewed Oct 31, 2024
View reviewed changes
src/main/java/net/snowflake/ingest/streaming/internal/ParquetFlusher.java Outdated
String filePath, long chunkStartOffset, Map<String, String> metadata) {
// We insert the filename in the file itself as metadata so that streams can work on replicated
// mixed tables. For a more detailed discussion on the topic see SNOW-561447 and
// http://go/streams-on-replicated-mixed-tables
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: pl add the other golink here too from line 133 in old code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

sfc-gh-hmadan
sfc-gh-hmadan reviewed Oct 31, 2024
View reviewed changes
src/main/java/net/snowflake/ingest/streaming/internal/ParquetFlusher.java
: Constants.PRIMARY_FILE_ID_KEY,
StreamingIngestUtils.getShortname(filePath));
} else {
String shortName = StreamingIngestUtils.getShortname(filePath);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets also assert / validate that enableIcebergStreaming is false when we're in the else path?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

sfc-gh-hmadan
sfc-gh-hmadan reviewed Oct 31, 2024
View reviewed changes
src/main/java/net/snowflake/ingest/streaming/internal/ParquetRowBuffer.java
@@ -89,6 +91,7 @@ public void setupSchema(List<ColumnMetadata> columns) {
if (!clientBufferParameters.isEnableIcebergStreaming()) {
metadata.put("sfVer", "1,1");
}
metadata.put(Constants.SDK_VERSION_KEY, RequestBuilder.DEFAULT_VERSION);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, Alec and I had discussed doing precisely this just yesterday!

We're going to have multiple SDK languages in a few months, should we have a slightly more verbose key-value pair here, such as:
userAgent=RequestBuilder.USER_AGENT ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User-Agent is a very HTTP-ish thing, we could name the key createdBy instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This current key-value is what XP expects. Let's iterate on it later?

@sfc-gh-asen sfc-gh-asen self-assigned this Oct 31, 2024
@sfc-gh-psaha sfc-gh-psaha force-pushed the psaha-unrevert-interleaved branch from a77c300 to 6dcad1e Compare October 31, 2024 18:05
@sfc-gh-psaha sfc-gh-psaha enabled auto-merge (squash) October 31, 2024 19:32
@sfc-gh-psaha sfc-gh-psaha force-pushed the psaha-unrevert-interleaved branch from d6a4513 to 52b1335 Compare October 31, 2024 20:27
@sfc-gh-psaha sfc-gh-psaha merged commit 8cbf2da into master Oct 31, 2024
47 checks passed
@sfc-gh-psaha sfc-gh-psaha deleted the psaha-unrevert-interleaved branch October 31, 2024 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-asen sfc-gh-asen sfc-gh-asen approved these changes

@sfc-gh-xhuang sfc-gh-xhuang sfc-gh-xhuang approved these changes

@sfc-gh-tzhang sfc-gh-tzhang sfc-gh-tzhang approved these changes

@sfc-gh-azagrebin sfc-gh-azagrebin Awaiting requested review from sfc-gh-azagrebin

@sfc-gh-alhuang sfc-gh-alhuang Awaiting requested review from sfc-gh-alhuang

@sfc-gh-hmadan sfc-gh-hmadan Awaiting requested review from sfc-gh-hmadan

Assignees

@sfc-gh-asen sfc-gh-asen

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sfc-gh-psaha @sfc-gh-tzhang @sfc-gh-xhuang @sfc-gh-hmadan @sfc-gh-asen