SNOW-1229108 Add warning about buffer config when single buffer enabled #896

sfc-gh-mbobowski · 2024-08-01T14:34:48Z

Overview

SNOW-1229108

I usually avoid mixing refactoring with adding logic but I didn't expect that it would be that much code.
I added comments to make your review easier but what I tried to achieve is:

move config validation logic outside of Utils / StreamingUtils classes
create separate config validation path for single and double buffer
add warning logs about some parameters being skipped for single buffer

Pre-review checklist

sfc-gh-mbobowski · 2024-08-01T14:37:12Z

src/main/java/com/snowflake/kafka/connector/DefaultConnectorConfigValidator.java

+import java.util.Map;
+import org.apache.kafka.common.config.ConfigException;
+
+public class DefaultConnectorConfigValidator implements ConnectorConfigValidator {


This class contains everything from the Utils.java validation.
This refactor is safe beacause of ConnectorConfigValidatorTest.

sfc-gh-mbobowski · 2024-08-01T14:41:50Z

...n/java/com/snowflake/kafka/connector/internal/streaming/DefaultStreamingConfigValidator.java

+import java.util.Set;
+import org.apache.kafka.common.config.ConfigException;
+
+public class DefaultStreamingConfigValidator implements StreamingConfigValidator {


This class contains everything from the StreamingUtils.java validation. The only difference is adding SingleBufferConfigValidator that logs warnings and DoubleBufferConfigValidator that performs validations as before.

It would be great to unit test this class separately from ConnectorConfigValidatorTest but I won't do it in this PR.

sfc-gh-mbobowski · 2024-08-01T14:43:20Z

src/test/java/com/snowflake/kafka/connector/ConnectorConfigValidatorLogsTest.java

+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+public class ConnectorConfigValidatorLogsTest {


This is refactored SecurityTest with one more test added.

sfc-gh-mbobowski · 2024-08-01T14:47:05Z

src/test/java/com/snowflake/kafka/connector/ConnectorConfigValidatorLogsTest.java

+
+ @ParameterizedTest
+ @ValueSource(strings = {BUFFER_FLUSH_TIME_SEC, BUFFER_SIZE_BYTES, BUFFER_COUNT_RECORDS})
+ public void shouldLogWarningIfBufferingPropertiesDefinedForSingleBuffer(String parameter)


I can't test the opposite behaviour because the log file is being appended. Clearly it's a bad design to do it this way instead of passing in memory logger... but I can't fix everything within a single PR.

If you think it is very bad I can just delete this test.

sfc-gh-mbobowski · 2024-08-01T15:25:17Z

Close / reopen to test snyk

sfc-gh-wtrefon · 2024-08-02T05:08:09Z

...n/java/com/snowflake/kafka/connector/internal/streaming/DefaultStreamingConfigValidator.java

+ Set<String> ignoredParameters =
+ new HashSet<>(
+ Arrays.asList(BUFFER_FLUSH_TIME_SEC, BUFFER_SIZE_BYTES, BUFFER_COUNT_RECORDS));
+ ignoredParameters.forEach(


nit: could be done in more streaming way
ignoredParameters.stream().filter(param -> config.containsKey(param)).forEach(param -> LOGGER...)

Great idea thx!

sfc-gh-wtrefon · 2024-08-02T05:08:44Z

...n/java/com/snowflake/kafka/connector/internal/streaming/DefaultStreamingConfigValidator.java

+
+ private void logDoubleBufferingParametersWarning(Map<String, String> config) {
+ if (InternalBufferParameters.isSingleBufferEnabled(config)) {
+ Set<String> ignoredParameters =


a list is enough, it does not need to be a set, we just iterate over it

Yeah, maybe it's too much to create a set here

sfc-gh-bzabek · 2024-08-02T08:12:08Z

src/main/java/com/snowflake/kafka/connector/internal/streaming/StreamingConfigValidator.java

+import java.util.Map;
+
+/** Validates connector config for Snowpipe Streaming */
+public interface StreamingConfigValidator {


Do you plan to create more validators? As for now It looks like these new interfaces are not really usefull.

I would say they might be useful for testing purpose.

…108-warn

…ed (snowflakedb#896) (cherry picked from commit 9aa0b0b)

* NOSNOW: added utility to upload connector to our nexus for simplified k8s deployment (snowflakedb#874) (cherry picked from commit ee83e22) * SNOW-1514185: Do assign the reopened channel unless kafka offsets are fully reset (snowflakedb#875) (cherry picked from commit 05dcbdf) * SNOW-1061851: Run SnowflakeSinkServiceV2IT for a single buffer (snowflakedb#876) (cherry picked from commit 7fce0f5) * SNOW-1061855 Fix the E2E test for SchemaEvolutionDropTable with a single buffer (snowflakedb#882) (cherry picked from commit fbd15d6) * NOSNOW: improve troubleshooting experience for scenario with skipped offsets (snowflakedb#883) (cherry picked from commit 7172481) * Adding logs to track schematization (snowflakedb#884) (cherry picked from commit 9905c7f) * Remove insertRows DEBUG log (called once per row) (snowflakedb#886) (cherry picked from commit 363457d) * NO-SNOW Upgrade nexus-staging-maven-plugin version (snowflakedb#887) (cherry picked from commit ac1276e) * Upgrade com.google.protobuf to 3.24.4 (snowflakedb#892) (cherry picked from commit 3c7e18c) * SNOW-1229110 Add single buffer usage data to telemetry (snowflakedb#893) (cherry picked from commit 96d3bfa) * SNOW-1541942 Extend Snowpipe initialization logging at INFO level (snowflakedb#885) (cherry picked from commit eab4c9c) * SNOW-947731 Remove deprecated avro-python3 package (snowflakedb#889) (cherry picked from commit 47624f5) * Bump dev.failsafe:failsafe from 3.2.1 to 3.3.2 (snowflakedb#859) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Michał Bobowski <michal.bobowski@snowflake.com> (cherry picked from commit 4a22f56) * Bump org.assertj:assertj-core from 3.25.3 to 3.26.3 (snowflakedb#881) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Michał Bobowski <michal.bobowski@snowflake.com> (cherry picked from commit adb896c) * Bump io.dropwizard.metrics:metrics-core from 4.2.3 to 4.2.26 (snowflakedb#863) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Michał Bobowski <michal.bobowski@snowflake.com> (cherry picked from commit 258ed2d) * NO-SNOW Ignore OAuth test (snowflakedb#906) (cherry picked from commit 30d2e8b) * SNOW-1229108 Add warning about buffer config when single buffer enabled (snowflakedb#896) (cherry picked from commit 9aa0b0b) * NO_SNOW Update dependencies (snowflakedb#898) Co-authored-by: Michał Bobowski <michal.bobowski@snowflake.com> (cherry picked from commit eff23e2) * SNOW-1618257 - upgrade to 2.2.0 ingest-sdk (snowflakedb#910) (cherry picked from commit d0e97fb) * NO-SNOW Release v.2.4.0 (snowflakedb#907) Co-authored-by: Xin Huang <xin.huang@snowflake.com> (cherry picked from commit 05c1148) --------- Co-authored-by: Greg Jachimko <greg.jachimko@snowflake.com> Co-authored-by: Adrian Kowalczyk <85181931+sfc-gh-akowalczyk@users.noreply.github.com> Co-authored-by: Michał Bobowski <145468486+sfc-gh-mbobowski@users.noreply.github.com> Co-authored-by: Xin Huang <xin.huang@snowflake.com> Co-authored-by: Wojciech Trefon <wojciech.trefon@snowflake.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

sfc-gh-mbobowski added 4 commits July 31, 2024 15:33

SNOW-1229108 Refactor config validation

8bccd0f

SNOW-1229108 Format.sh

71842d2

SNOW-1229108 Add warning about double buffering parameters

fcc090f

SNOW-1229108 Change interface naming convention

6192188

sfc-gh-mbobowski requested a review from a team as a code owner August 1, 2024 14:34

SNOW-1229108 Format.sh

74523ff

sfc-gh-mbobowski commented Aug 1, 2024

View reviewed changes

sfc-gh-mbobowski closed this Aug 1, 2024

sfc-gh-mbobowski reopened this Aug 1, 2024

sfc-gh-wtrefon reviewed Aug 2, 2024

View reviewed changes

sfc-gh-wtrefon approved these changes Aug 2, 2024

View reviewed changes

sfc-gh-bzabek reviewed Aug 2, 2024

View reviewed changes

sfc-gh-mbobowski added 3 commits August 5, 2024 10:27

SNOW-1229108 PR fixes

7ae214d

SNOW-1229108 Adjust log Level

c87e84c

Merge remote-tracking branch 'origin/master' into mbobowski-SNOW-1229…

3a5bc88

…108-warn

sfc-gh-mbobowski merged commit 9aa0b0b into master Aug 8, 2024
77 of 80 checks passed

sfc-gh-mbobowski deleted the mbobowski-SNOW-1229108-warn branch August 8, 2024 10:53

airlock-confluentinc bot pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Aug 22, 2024

SNOW-1229108 Add warning about buffer config when single buffer enabl…

aac5172

…ed (snowflakedb#896) (cherry picked from commit 9aa0b0b)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-1229108 Add warning about buffer config when single buffer enabled #896

SNOW-1229108 Add warning about buffer config when single buffer enabled #896

sfc-gh-mbobowski commented Aug 1, 2024 •

edited

Loading

sfc-gh-mbobowski Aug 1, 2024 •

edited

Loading

sfc-gh-mbobowski Aug 1, 2024

sfc-gh-mbobowski Aug 1, 2024

sfc-gh-mbobowski Aug 1, 2024

sfc-gh-mbobowski commented Aug 1, 2024

sfc-gh-wtrefon Aug 2, 2024

sfc-gh-mbobowski Aug 5, 2024

sfc-gh-wtrefon Aug 2, 2024

sfc-gh-mbobowski Aug 5, 2024

sfc-gh-bzabek Aug 2, 2024

sfc-gh-mbobowski Aug 5, 2024

SNOW-1229108 Add warning about buffer config when single buffer enabled #896

SNOW-1229108 Add warning about buffer config when single buffer enabled #896

Conversation

sfc-gh-mbobowski commented Aug 1, 2024 • edited Loading

Overview

Pre-review checklist

sfc-gh-mbobowski Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-mbobowski commented Aug 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-mbobowski commented Aug 1, 2024 •

edited

Loading

sfc-gh-mbobowski Aug 1, 2024 •

edited

Loading