SNOW-979849: Support user to pass a user defined offset token verification logic as part of channel creation #680

sfc-gh-tzhang · 2024-02-21T18:48:48Z

With this PR, user could pass in a user defined offset token verification function that is used to verify offset behaviors, if the verification failed, we will log an error and report it to SF

…-ingest-java into tzhang-si-overflow

src/main/java/net/snowflake/ingest/streaming/internal/AbstractRowBuffer.java

sfc-gh-psaha · 2024-02-22T21:22:39Z

src/main/java/net/snowflake/ingest/connection/TelemetryService.java

@@ -66,6 +69,8 @@ public String toString() {
  TelemetryService(CloseableHttpClient httpClient, String clientName, String url) {
    this.clientName = clientName;
    this.telemetry = (TelemetryClient) TelemetryClient.createSessionlessTelemetry(httpClient, url);
+    // At most once every seconds


Suggested change

// At most once every seconds

// At most once every second

sfc-gh-psaha · 2024-02-22T21:28:26Z

src/main/java/net/snowflake/ingest/streaming/OffsetTokenVerificationFunction.java

+package net.snowflake.ingest.streaming;
+
+/**
+ * Interface to create a user defined offset token verification function that is used to verify


Suggested change

* Interface to create a user defined offset token verification function that is used to verify

* Interface to provide a custom offset verification logic. If specified, verification failures will be logged as warnings and reported to Snowflake.

sfc-gh-psaha · 2024-02-22T21:29:36Z

src/main/java/net/snowflake/ingest/streaming/OffsetTokenVerificationFunction.java

+ * Interface to create a user defined offset token verification function that is used to verify
+ * offset behaviors, if the verification failed, we will log a warning and report it to SF
+ *
+ * <p>Below is an example that verifies all offset tokens need to monotonically increment numbers:


Suggested change

* <p>Below is an example that verifies all offset tokens need to monotonically increment numbers:

* <p>Below is an example that verifies that all offset tokens are monotonically increasing numbers:

sfc-gh-psaha · 2024-02-22T21:30:36Z

src/main/java/net/snowflake/ingest/streaming/OffsetTokenVerificationFunction.java

+ *               }
+ *             }
+ *           } catch (NumberFormatException ignored) {
+ *             // Do nothing since we can't compare the offset


nit: One should probably report a mismatch if offset is not of the right format.

We actually allow offset token to be any arbitrary String

I get that. I was only referring to this specific example where the user knows that it should be a parseable as a number.

sfc-gh-psaha · 2024-02-22T21:33:18Z

src/main/java/net/snowflake/ingest/streaming/internal/AbstractRowBuffer.java

@@ -669,6 +694,30 @@ private void checkBatchSizeRecommendedMaximum(float batchSizeInBytes) {
    }
  }

+  /**
+   * We verify some offset expect behaviors based on the provided verification logic and report to


Suggested change

* We verify some offset expect behaviors based on the provided verification logic and report to

* We verify the offset based on the provided verification logic and report to

sfc-gh-psaha · 2024-02-22T21:34:21Z

src/main/java/net/snowflake/ingest/streaming/internal/AbstractRowBuffer.java

+   */
+  private void checkOffsetMismatch(
+      String prevEndOffset, String curStartOffset, String curEndOffset, int rowCount) {
+    if (telemetryService != null


I think we should still do the verification even if there is no telemetry service. You can check the service before using it. Some users may want the warning even if they don't want anything sent to Snowflake.

sfc-gh-psaha · 2024-02-22T21:36:34Z

src/main/java/net/snowflake/ingest/streaming/OffsetTokenVerificationFunction.java

+
+package net.snowflake.ingest.streaming;
+
+/**


I think we should provide some guidance around when this should be used and what to do if users notice mismatches etc. Okay to point to Snowflake docs.

Added a sentence, PTAL, I don't think we have any documentation on this.

…-ingest-java into tzhang-si-overflow

sfc-gh-gdoci · 2024-02-23T11:09:19Z

src/test/java/net/snowflake/ingest/streaming/internal/StreamingIngestIT.java

@@ -78,6 +79,35 @@ public static Object[] compressionAlgorithms() {

  @Parameter public String compressionAlgorithm;

+  private static final OffsetTokenVerificationFunction offsetTokenVerificationFunction =


Should we verify the behaviour of the this verification function (since it's the one we recommend to users) by test cases that have/don't have a mismatch?

added a small test

sfc-gh-psaha · 2024-02-23T18:47:39Z

src/main/java/net/snowflake/ingest/streaming/OffsetTokenVerificationFunction.java

- * offset behaviors, if the verification failed, we will log a warning and report it to SF
+ * Interface to provide a custom offset verification logic. If specified, verification failures will
+ * be logged as warnings and reported to Snowflake. This interface could be used when there are
+ * certain assumption about the offset token behavior and please reach out to Snowflake if you


Suggested change

* certain assumption about the offset token behavior and please reach out to Snowflake if you

* certain assumption about the offset token behavior. Please reach out to Snowflake if you

sfc-gh-japatel · 2024-02-23T23:58:44Z

src/main/java/net/snowflake/ingest/connection/TelemetryService.java

@@ -66,6 +69,8 @@ public String toString() {
  TelemetryService(CloseableHttpClient httpClient, String clientName, String url) {
    this.clientName = clientName;
    this.telemetry = (TelemetryClient) TelemetryClient.createSessionlessTelemetry(httpClient, url);
+    // At most once every second
+    this.rateLimiter = RateLimiter.create(1.0);


isnt this too low? essentially it is one per second across all channels IIUC?

Good point, what about we create one RateLimiter per channel?

yeah.. one per channel is okay!

src/main/java/net/snowflake/ingest/connection/TelemetryService.java

src/main/java/net/snowflake/ingest/streaming/OffsetTokenVerificationFunction.java

sfc-gh-japatel

lgtm!
left a suggestion on ratelimiter.

src/main/java/net/snowflake/ingest/connection/TelemetryService.java

sfc-gh-tzhang added 18 commits February 2, 2024 06:42

fix overflow

a31a4b6

minor change

a5bce3d

Merge branch 'master' into tzhang-si-overflow

e365bb6

address comments

27ce579

Merge branch 'tzhang-si-overflow' of github.com:snowflakedb/snowflake…

aecd879

…-ingest-java into tzhang-si-overflow

Merge branch 'master' into tzhang-si-overflow

06cbdc3

log and verify

45808d7

add tests

b28e564

format

3d2971c

fix tests

de39f7e

update logic

1df51b4

update logic

89fa743

merge master

704a940

fix tests

d925dcb

support input function

cd3b970

Merge branch 'master' into tzhang-si-overflow

a426664

improve comment

61808a0

minor change

9241678

sfc-gh-tzhang requested review from a team as code owners February 21, 2024 18:48

sfc-gh-tzhang commented Feb 21, 2024

View reviewed changes

src/main/java/net/snowflake/ingest/streaming/internal/AbstractRowBuffer.java Outdated Show resolved Hide resolved

sfc-gh-tzhang added 4 commits February 21, 2024 21:02

fix tests

bb3b069

add rate limiter

781a87c

minor change

c0293f3

Merge branch 'master' into tzhang-si-overflow

17a3137

sfc-gh-xhuang approved these changes Feb 22, 2024

View reviewed changes

sfc-gh-xhuang requested review from sfc-gh-tjones and sfc-gh-psaha February 22, 2024 17:56

sfc-gh-psaha requested changes Feb 22, 2024

View reviewed changes

address comments

368b3b5

sfc-gh-tzhang added 3 commits February 23, 2024 00:52

Merge branch 'tzhang-si-overflow' of github.com:snowflakedb/snowflake…

2475bdf

…-ingest-java into tzhang-si-overflow

address comment

b9ab716

Merge branch 'master' into tzhang-si-overflow

11fc5a3

sfc-gh-tzhang requested a review from sfc-gh-psaha February 23, 2024 02:27

sfc-gh-gdoci reviewed Feb 23, 2024

View reviewed changes

sfc-gh-psaha approved these changes Feb 23, 2024

View reviewed changes

address comments

c77b976

sfc-gh-japatel reviewed Feb 23, 2024

View reviewed changes

src/main/java/net/snowflake/ingest/connection/TelemetryService.java Show resolved Hide resolved

sfc-gh-japatel reviewed Feb 24, 2024

View reviewed changes

src/main/java/net/snowflake/ingest/streaming/OffsetTokenVerificationFunction.java Show resolved Hide resolved

sfc-gh-japatel approved these changes Feb 24, 2024

View reviewed changes

sfc-gh-japatel reviewed Feb 24, 2024

View reviewed changes

src/main/java/net/snowflake/ingest/connection/TelemetryService.java Show resolved Hide resolved

sfc-gh-tzhang added 2 commits February 27, 2024 02:01

address comments

ecbc8d5

minor change

5e2636e

sfc-gh-tzhang merged commit a4f2eaf into master Feb 27, 2024
12 checks passed

sfc-gh-tzhang deleted the tzhang-si-overflow branch February 27, 2024 05:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-979849: Support user to pass a user defined offset token verification logic as part of channel creation #680

SNOW-979849: Support user to pass a user defined offset token verification logic as part of channel creation #680

sfc-gh-tzhang commented Feb 21, 2024

sfc-gh-psaha Feb 22, 2024

sfc-gh-psaha Feb 22, 2024

sfc-gh-psaha Feb 22, 2024

sfc-gh-psaha Feb 22, 2024

sfc-gh-tzhang Feb 23, 2024

sfc-gh-psaha Feb 23, 2024

sfc-gh-psaha Feb 22, 2024

sfc-gh-psaha Feb 22, 2024

sfc-gh-psaha Feb 22, 2024

sfc-gh-tzhang Feb 23, 2024

sfc-gh-gdoci Feb 23, 2024 •

edited

Loading

sfc-gh-tzhang Feb 23, 2024

sfc-gh-psaha Feb 23, 2024

sfc-gh-japatel Feb 23, 2024

sfc-gh-tzhang Feb 24, 2024

sfc-gh-japatel Feb 24, 2024

sfc-gh-japatel left a comment

	* Interface to create a user defined offset token verification function that is used to verify
	* Interface to provide a custom offset verification logic. If specified, verification failures will be logged as warnings and reported to Snowflake.

	* <p>Below is an example that verifies all offset tokens need to monotonically increment numbers:
	* <p>Below is an example that verifies that all offset tokens are monotonically increasing numbers:

	* We verify some offset expect behaviors based on the provided verification logic and report to
	* We verify the offset based on the provided verification logic and report to

		@@ -78,6 +79,35 @@ public static Object[] compressionAlgorithms() {

		@Parameter public String compressionAlgorithm;

		private static final OffsetTokenVerificationFunction offsetTokenVerificationFunction =

	* certain assumption about the offset token behavior and please reach out to Snowflake if you
	* certain assumption about the offset token behavior. Please reach out to Snowflake if you

SNOW-979849: Support user to pass a user defined offset token verification logic as part of channel creation #680

SNOW-979849: Support user to pass a user defined offset token verification logic as part of channel creation #680

Conversation

sfc-gh-tzhang commented Feb 21, 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

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-gdoci Feb 23, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-japatel left a comment

Choose a reason for hiding this comment

sfc-gh-gdoci Feb 23, 2024 •

edited

Loading