-
Notifications
You must be signed in to change notification settings - Fork 98
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.
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
Revert "NO-SNOW Enable single buffering in Snowpipe Streaming by default (#924)" #981
Revert "NO-SNOW Enable single buffering in Snowpipe Streaming by default (#924)" #981
Conversation
@@ -495,7 +495,6 @@ public void testStreamingEmptyFlushTime() { | |||
IngestionMethodConfig.SNOWPIPE_STREAMING.toString()); | |||
config.put(Utils.SF_ROLE, "ACCOUNTADMIN"); | |||
config.remove(SnowflakeSinkConnectorConfig.BUFFER_FLUSH_TIME_SEC); | |||
config.put(SNOWPIPE_STREAMING_ENABLE_SINGLE_BUFFER, "false"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could keep that, it will be later easier to set the default back to true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, just consider keeping config.put(SNOWPIPE_STREAMING_ENABLE_SINGLE_BUFFER, "false");
in the tests
@@ -133,7 +133,7 @@ public class SnowflakeSinkConnectorConfig { | |||
public static final String SNOWPIPE_STREAMING_ENABLE_SINGLE_BUFFER = | |||
"snowflake.streaming.enable.single.buffer"; | |||
|
|||
public static final boolean SNOWPIPE_STREAMING_ENABLE_SINGLE_BUFFER_DEFAULT = true; | |||
public static final boolean SNOWPIPE_STREAMING_ENABLE_SINGLE_BUFFER_DEFAULT = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we decided not to introduce / change default values of the SDK flush timeout / flush size?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in 2.5.0. We'll try to figure this out for the next release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, it's a revert 😓
This reverts commit f0e5f23.
Overview
We decided to not enable this setting by default in 2.5.0 release because we are not sure what is the impact on the first read performance.
Pre-review checklist
snowflake.ingestion.method
.Yes
- Added end to end and Unit Tests.No
- Suggest why it is not param protected