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

NOSNOW: improve troubleshooting experience for scenario with skipped offsets #883

Merged
merged 1 commit into from
Jul 18, 2024
Merged

NOSNOW: improve troubleshooting experience for scenario with skipped offsets #883

merged 1 commit into from
Jul 18, 2024

Conversation

sfc-gh-gjachimko
Copy link
Contributor

NOSNOW

Change the verbosity (of first log message occurrence) and message content to improve troubleshooting expience as requested by support team.

Pre-review checklist

  • This change should be part of a Behavior Change Release. See go/behavior-change.
  • This change has passed Merge gate tests
  • Snowpipe Changes
  • Snowpipe Streaming Changes
  • This change is TEST-ONLY
  • This change is README/Javadocs only
  • This change is protected by a config parameter <PARAMETER_NAME> eg snowflake.ingestion.method.
    • Yes - Added end to end and Unit Tests.
    • No - Suggest why it is not param protected
  • Is his change protected by parameter <PARAMETER_NAME> on the server side?
    • The parameter/feature is not yet active in production (partial rollout or PrPr, see Changes for Unreleased Features and Fixes).
    • If there is an issue, it can be safely mitigated by turning the parameter off. This is also verified by a test (See go/ppp).

@sfc-gh-gjachimko sfc-gh-gjachimko requested a review from a team as a code owner July 16, 2024 07:24
sfc-gh-achyzy
sfc-gh-achyzy reviewed Jul 16, 2024
View reviewed changes
Copy link
Contributor

@sfc-gh-achyzy sfc-gh-achyzy left a comment

Choose a reason for hiding this comment

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

I'm not sure I like the usage of different levels depending on the number of occurrences.

src/main/java/com/snowflake/kafka/connector/internal/streaming/DirectTopicPartitionChannel.java Outdated
@@ -323,14 +328,20 @@ public void insertRecord(SinkRecord kafkaSinkRecord, boolean isFirstRowPerPartit
if (currentProcessedOffset == NO_OFFSET_TOKEN_REGISTERED_IN_SNOWFLAKE
|| kafkaSinkRecord.kafkaOffset() >= currentProcessedOffset + 1) {
transformAndSend(kafkaSinkRecord);
isFirstMismatchedOffset.set(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of logging something on the first occurrence and later skipping.
I propose to keep it consistent.

Copy link
Contributor Author

@sfc-gh-gjachimko sfc-gh-gjachimko Jul 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

after discussions - changed to warn.

src/main/java/com/snowflake/kafka/connector/internal/streaming/DirectTopicPartitionChannel.java Outdated
isFirstMismatchedOffset.getAndSet(false) ? LOGGER::warn : LOGGER::debug;

logMsgDelegate.accept(
"Connector's channel {} expected record at offset {} but received {}. Offset stored in"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Connector's channel {} expected record at offset {} but received {}. Offset stored in"
"Channel {} expected record at offset {} but received {}. The current offset stored in"

sfc-gh-hachouraria
sfc-gh-hachouraria reviewed Jul 17, 2024
View reviewed changes
...ain/java/com/snowflake/kafka/connector/internal/streaming/BufferedTopicPartitionChannel.java Outdated
+ " offsetPersistedInSnowflake:{}, processedOffset:{}",
kafkaSinkRecord.kafkaOffset(),
LOGGER.warn(
"Channel {} expected record at offset {} but received {}. The current offset stored in"
Copy link

@sfc-gh-hachouraria sfc-gh-hachouraria Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thank you very much for addressing this!

Could we keep the skip messaging too?

The new warn message is great, but it doesn't let the reader know what the connector's action was (skip) so they can mentally match it to their observed lag/loss situation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, extended the message a bit.

@sfc-gh-gjachimko sfc-gh-gjachimko force-pushed the gjachimko/NOSNOW-improve_skip_offset_logging branch from 0de3fad to 829d568 Compare July 18, 2024 06:20
@sfc-gh-gjachimko sfc-gh-gjachimko merged commit 7172481 into master Jul 18, 2024
81 checks passed
@sfc-gh-gjachimko sfc-gh-gjachimko deleted the gjachimko/NOSNOW-improve_skip_offset_logging branch July 18, 2024 08:46
airlock-confluentinc bot pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Aug 22, 2024
@sfc-gh-gjachimko @sangeet259
NOSNOW: improve troubleshooting experience for scenario with skipped …
0dc8ca0 
…offsets (snowflakedb#883)

(cherry picked from commit 7172481)
sangeet259 added a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Aug 22, 2024
@sangeet259 @sfc-gh-gjachimko
@sfc-gh-akowalczyk @sfc-gh-mbobowski @sfc-gh-xhuang @sfc-gh-wtrefon @dependabot
Release bump 2 4 0 (#63)
f1067de 
* 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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Michał Bobowski <[email protected]>
(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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Michał Bobowski <[email protected]>
(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] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Michał Bobowski <[email protected]>
(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 <[email protected]>
(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 <[email protected]>
(cherry picked from commit 05c1148)


---------

Co-authored-by: Greg Jachimko <[email protected]>
Co-authored-by: Adrian Kowalczyk <[email protected]>
Co-authored-by: Michał Bobowski <[email protected]>
Co-authored-by: Xin Huang <[email protected]>
Co-authored-by: Wojciech Trefon <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-achyzy sfc-gh-achyzy sfc-gh-achyzy left review comments

@sfc-gh-hachouraria sfc-gh-hachouraria sfc-gh-hachouraria left review comments

@sfc-gh-lshcharbaty sfc-gh-lshcharbaty sfc-gh-lshcharbaty approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sfc-gh-gjachimko @sfc-gh-achyzy @sfc-gh-hachouraria @sfc-gh-lshcharbaty