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

NO-SNOW Improve offset logging for Snowpipe #1027

Merged
merged 10 commits into from
Dec 11, 2024
Merged

NO-SNOW Improve offset logging for Snowpipe #1027

merged 10 commits into from
Dec 11, 2024

Conversation

sfc-gh-mbobowski
Copy link
Contributor

@sfc-gh-mbobowski sfc-gh-mbobowski commented Dec 10, 2024 •
edited
Loading

Overview

Refactored from:
#1023

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-mbobowski sfc-gh-mbobowski requested a review from a team as a code owner December 10, 2024 12:44
@sfc-gh-mbobowski sfc-gh-mbobowski changed the title NO-SNOW Improce offset logging for file cleaner NO-SNOW Improve offset logging for file cleaner Dec 10, 2024
@sfc-gh-mbobowski sfc-gh-mbobowski changed the title NO-SNOW Improve offset logging for file cleaner NO-SNOW Improve offset logging for Snowpipe Dec 10, 2024
sfc-gh-wtrefon
sfc-gh-wtrefon reviewed Dec 10, 2024
View reviewed changes
src/main/java/com/snowflake/kafka/connector/internal/OffsetContinuityRanges.java
import org.apache.commons.lang3.tuple.Pair;

public class OffsetContinuityRanges {
private final List<Pair<Long, Long>> continuousOffsets;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we could also introduce a class "OffsetRange" with startOffsetInclusive, endOffsetExclusive but I'm fine with a pair

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or a data class... oh no wait, it's not a Kotlin.
I think that Pair is good enough for this use case.

sfc-gh-wtrefon
sfc-gh-wtrefon reviewed Dec 10, 2024
View reviewed changes
src/test/java/com/snowflake/kafka/connector/internal/FileNameUtilsTest.java
Arguments.of(Collections.singletonList(fileName(filePrefix, 0, 10)), "[[0,10]]", "[]"),
Arguments.of(
Arrays.asList(fileName(filePrefix, 0, 10), fileName(filePrefix, 100, 2137)),
"[[0,10][100,2137]]",
Copy link
Contributor

Choose a reason for hiding this comment

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

✝️

sfc-gh-wtrefon
sfc-gh-wtrefon reviewed Dec 10, 2024
View reviewed changes
src/main/java/com/snowflake/kafka/connector/internal/SnowflakeSinkServiceV1.java Outdated
LOGGER.debug(
"Moving failed files for pipe:{} to tableStage failedFileCount:{}, failedFiles:{}",
OffsetContinuityRanges offsets = searchForMissingOffsets(failedFiles);
LOGGER.info(
Copy link
Contributor

Choose a reason for hiding this comment

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

could we also print failedFiles on debug lvl? The original PR had that and I think this can be useful to know the prefixes when debbuging some cases

@sfc-gh-wtrefon sfc-gh-wtrefon enabled auto-merge (squash) December 10, 2024 13:45
@sfc-gh-wtrefon sfc-gh-wtrefon merged commit c8e6df9 into master Dec 11, 2024
53 of 54 checks passed
@sfc-gh-wtrefon sfc-gh-wtrefon deleted the mbobowski-NO-SNOW-offset-logging branch December 11, 2024 16:59
airlock-confluentinc bot pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Dec 19, 2024
@sfc-gh-mbobowski @sfc-gh-wtrefon @sudeshwasnik
NO-SNOW Improve offset logging for Snowpipe (snowflakedb#1027)
95471f2 
Co-authored-by: Wojciech Trefon <[email protected]>
sudeshwasnik added a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Dec 20, 2024
@sudeshwasnik @sfc-gh-mbobowski @sfc-gh-wtrefon
NO-SNOW Improve offset logging for Snowpipe (snowflakedb#1027) (#73)
ecc4fe4 
Co-authored-by: Michał Bobowski <[email protected]>
Co-authored-by: Wojciech Trefon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sfc-gh-wtrefon sfc-gh-wtrefon sfc-gh-wtrefon approved these changes

@sfc-gh-dseweryn sfc-gh-dseweryn sfc-gh-dseweryn 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.
3 participants
@sfc-gh-mbobowski @sfc-gh-dseweryn @sfc-gh-wtrefon