SNOW-1437885 Disable blob interleaving when in Iceberg mode #763

sfc-gh-alhuang · 2024-05-23T00:29:07Z

Added a method to SnowflakeStreamingIngestClientInternal to activate/deactivate streaming to Iceberg tables.
A blob should only contain one chunk (parquet) under Iceberg mode. Disable blob interleaving under Iceberg mode.

sfc-gh-gdoci · 2024-05-24T09:13:39Z

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

@@ -123,6 +123,10 @@ List<List<ChannelData<T>>> getData() {
  // blob encoding version
  private final Constants.BdecVersion bdecVersion;

+  // Indicates if it's flushing to Iceberg tables, a blob could only contain one chunk under Iceberg
+  // mode
+  private final boolean isIcebergMode;


Can this have a more general name, e.g. isNonInterleavedMode or disableInterleavedBlobs?

Yeah, we can make this more general for the internal names since we disable interleaved mode due to other reasons in the future, not only Iceberg

Removed the non-interleave parameter for now as we could control this via MAX_CHUNKS_IN_BLOB_AND_REGISTRATION_REQUEST

sfc-gh-hmadan · 2024-05-24T16:51:31Z

...ain/java/net/snowflake/ingest/streaming/internal/SnowflakeStreamingIngestClientInternal.java

  }

  /*** Constructor for TEST ONLY
   *
   * @param name the name of the client
   */
  SnowflakeStreamingIngestClientInternal(String name) {
-    this(name, null, null, null, true, null, new HashMap<>());
+    this(name, null, null, null, false, true, null, new HashMap<>());


there's about 20 testcases using this ctor, I'd want to get iceberg mode test coverage for all those tests too (unless there's a good reason to not need it).
Can you expose isIcebergMode on this test mode ctor's signature too, and parameterize the calling tests to run with both icebergMode = on / off?

I see we're using JUnit which has good support for doing multiple test runs with different parameter values declared on an annotation / on a values provider defined as a test class member, so it'll be a one-time cost to set this up in all the test classes, but in exchange we'll get comprehensive ongoing test coverage.

cc @sfc-gh-tzhang in you case you want to weigh in on this!

agree, you can search for Parameterized and see how to use that

Added parameters for unit tests.

sfc-gh-hmadan · 2024-05-24T17:12:22Z

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

@@ -437,7 +447,7 @@ && shouldStopProcessing(
          }
          // Add processed channels to the current blob, stop if we need to create a new blob
          blobData.add(channelsDataPerTable.subList(0, idx));
-          if (idx != channelsDataPerTable.size()) {
+          if (idx != channelsDataPerTable.size() || isIcebergMode) {


I want to guard against the possibility of having isIcebergMode peppered across 10 different places in code, making it that much harder to maintain the client.

Ideally we should have one place in the whole client that does if (isIcebergMode) { /* initialize booleans / ints / settings one way */ } else { /* initialize another way */ } - it seems that the ParameterProvider is the best place to do this, or we introduce a separate ClientSettings class that holds all these settings that control different behaviors in different places in the client.

I also see there are three parameters on the parameter provider that need different values for iceberg - MAX_CHUNK_SIZE_IN_BYTES (set to 512 MB rn) and MAX_CHUNKS_IN_BLOB_AND_REGISTRATION_REQUEST_DEFAULT (set to 100 rn) and MAX_BLOB_SIZE_IN_BYTES (set to 1 GB rn). This is in addition to MAX_CLIENT_LAG needing an iceberg-specific value.

It looks like if parameterProvider.getMaxChunksInBlobAndRegistrationRequest() returns 1 then you'll get this same desired behavior ?

Note - in the parameterProvider you'll need to validate that customers can't override MAX_CHUNKS_IN_BLOB_AND_REGISTRATION_REQUEST and it can only ever be 1.

Deleted the isIcebergMode param and use MAX_CHUNKS_IN_BLOB_AND_REGISTRATION_REQUEST for now.

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

sfc-gh-hmadan

Left a couple comments, PTAL.

sfc-gh-tzhang

I think there're a few things missing in this PR:

Block creation of regular channels on Iceberg client and creation of iceberg channels on regular client
No need to call the client/configure endpoint for Iceberg client

Do you plan to do that in a separate PR?

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

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

sfc-gh-tzhang · 2024-05-24T21:34:43Z

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

@@ -123,6 +123,10 @@ List<List<ChannelData<T>>> getData() {
  // blob encoding version
  private final Constants.BdecVersion bdecVersion;

+  // Indicates if it's flushing to Iceberg tables, a blob could only contain one chunk under Iceberg
+  // mode
+  private final boolean isIcebergMode;


Yeah, we can make this more general for the internal names since we disable interleaved mode due to other reasons in the future, not only Iceberg

sfc-gh-tzhang · 2024-05-24T21:42:43Z

...ain/java/net/snowflake/ingest/streaming/internal/SnowflakeStreamingIngestClientInternal.java

  }

  /*** Constructor for TEST ONLY
   *
   * @param name the name of the client
   */
  SnowflakeStreamingIngestClientInternal(String name) {
-    this(name, null, null, null, true, null, new HashMap<>());
+    this(name, null, null, null, false, true, null, new HashMap<>());


agree, you can search for Parameterized and see how to use that

sfc-gh-alhuang · 2024-05-28T20:45:59Z

Thanks for the comments and tips! Added icebergMode on/off params for some unit tests. Some tests included blob registration and authentication only test wit regular mode for now. @sfc-gh-tzhang I plan to delete or modify the client configure call in later PRs as it's only called by StreamingIngestStage and we might not use this class in the iceberg mode later. The Iceberg client & table compatibility check is adding in the server side (ref), should we also add a check in the client?

sfc-gh-hmadan · 2024-06-03T16:17:13Z

src/main/java/net/snowflake/ingest/utils/ParameterProvider.java

+    // Required parameter override for Iceberg mode
+    if (this.isIcebergMode) {
+      this.parameterMap.put(
+          MAX_CHUNKS_IN_BLOB_AND_REGISTRATION_REQUEST,


why not call this.updateValue here ? That's the only method that's updating parameterMap before this block was added, lets keep it that way..?

The updateValue considers props and paramOverrides before default value. While under iceberg mode we should not allow user to set the chunk number > 1, that's why I use the put method directly. I think we can use updateValue(key, value, null, null) to achieve the same result, wdyt?

sfc-gh-hmadan

LGTM, left one small comment that you can take in the next PR too as you have other changes waiting for this to go in.

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

sfc-gh-tzhang · 2024-06-05T22:03:20Z

src/main/java/net/snowflake/ingest/utils/ParameterProvider.java

+  public static final int MAX_CHUNKS_IN_BLOB_AND_REGISTRATION_REQUEST_ICEBERG_MODE_DEFAULT = 1;
+
+  // If the provided parameters need to be verified and modified to meet Iceberg mode
+  private final boolean isIcebergMode;


what happens if customer create a Iceberg client but override this value to false?

I don't think client can inject ParameterProvider directly, instantiate is called by ClientInternal at here.

src/main/java/net/snowflake/ingest/utils/ParameterProvider.java

sfc-gh-tzhang · 2024-06-05T22:26:46Z

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

 public class FlushServiceTest {
+
+  @Parameterized.Parameters(name = "isIcebergMode: {0}")


I thought what we should be doing is to test the isIcebergMode with {true, false}

This line is the format of the test name, where the first parameter is either false or true from here.

sfc-gh-alhuang · 2024-06-10T23:05:32Z

Per offline discussion with @sfc-gh-tzhang , moved MAX_CHUNKS_IN_BLOB_AND_REGISTRATION_REQUEST_ICEBERG_MODE_DEFAULT back to ParameterProvider.java.

sfc-gh-tzhang

Left some comments, PTAL, otherwise LGTM! One request I have is could we merge all the Iceberg related changes to a feature branch instead of main? We can move everything to main once we ready, otherwise the new release will contain all the WIP changes and customer might do something with it.

...ain/java/net/snowflake/ingest/streaming/internal/SnowflakeStreamingIngestClientInternal.java

src/main/java/net/snowflake/ingest/utils/ParameterProvider.java

…763)" This reverts commit c6dfbf3.

* Add parameters & disable interleaving mode

Add parameters & disable interleaving mode

1cb85e9

sfc-gh-alhuang marked this pull request as ready for review May 23, 2024 17:01

sfc-gh-alhuang requested review from sfc-gh-tzhang and a team as code owners May 23, 2024 17:01

sfc-gh-alhuang requested a review from sfc-gh-hmadan May 23, 2024 17:01

sfc-gh-gdoci reviewed May 24, 2024

View reviewed changes

sfc-gh-hmadan reviewed May 24, 2024

View reviewed changes

src/test/java/net/snowflake/ingest/streaming/internal/FlushServiceTest.java Outdated Show resolved Hide resolved

sfc-gh-hmadan requested changes May 24, 2024

View reviewed changes

sfc-gh-tzhang reviewed May 24, 2024

View reviewed changes

sfc-gh-alhuang added 2 commits May 25, 2024 00:10

Add parameter for tests & Iceberg mode

c4534b2

Add comment

6c9bc9b

sfc-gh-alhuang requested review from sfc-gh-tzhang, sfc-gh-hmadan and sfc-gh-gdoci May 28, 2024 20:46

sfc-gh-hmadan reviewed Jun 3, 2024

View reviewed changes

sfc-gh-hmadan approved these changes Jun 3, 2024

View reviewed changes

sfc-gh-tzhang reviewed Jun 5, 2024

View reviewed changes

Refactor.

2ef26a2

sfc-gh-alhuang requested a review from sfc-gh-tzhang June 6, 2024 20:34

fix parameter provider

022a2d8

sfc-gh-tzhang approved these changes Jun 10, 2024

View reviewed changes

Add parameter requirements

489307a

sfc-gh-alhuang changed the base branch from master to iceberg-support June 11, 2024 00:30

sfc-gh-alhuang merged commit c6dfbf3 into iceberg-support Jun 11, 2024
15 checks passed

sfc-gh-alhuang deleted the alhuang-iceberg-mode branch June 11, 2024 00:31

sfc-gh-alhuang added a commit that referenced this pull request Jun 11, 2024

Revert "SNOW-1437885 Disable blob interleaving when in Iceberg mode (#…

4cd1bea

…763)" This reverts commit c6dfbf3.

sfc-gh-alhuang added a commit that referenced this pull request Jul 10, 2024

SNOW-1437885 Disable blob interleaving when in Iceberg mode (#763)

2b26107

* Add parameters & disable interleaving mode

sfc-gh-alhuang mentioned this pull request Jul 26, 2024

SNOW-1483230 Parameter support & disable blob encryption for new table format #801

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-1437885 Disable blob interleaving when in Iceberg mode #763

SNOW-1437885 Disable blob interleaving when in Iceberg mode #763

sfc-gh-alhuang commented May 23, 2024

sfc-gh-gdoci May 24, 2024 •

edited

Loading

sfc-gh-tzhang May 24, 2024

sfc-gh-alhuang May 28, 2024

sfc-gh-hmadan May 24, 2024

sfc-gh-tzhang May 24, 2024

sfc-gh-alhuang May 28, 2024

sfc-gh-hmadan May 24, 2024

sfc-gh-alhuang May 28, 2024

sfc-gh-hmadan left a comment

sfc-gh-tzhang left a comment

sfc-gh-tzhang May 24, 2024

sfc-gh-tzhang May 24, 2024

sfc-gh-alhuang commented May 28, 2024 •

edited

Loading

sfc-gh-hmadan Jun 3, 2024

sfc-gh-alhuang Jun 3, 2024

sfc-gh-hmadan left a comment

sfc-gh-tzhang Jun 5, 2024

sfc-gh-alhuang Jun 5, 2024

sfc-gh-tzhang Jun 5, 2024

sfc-gh-alhuang Jun 5, 2024

sfc-gh-alhuang commented Jun 10, 2024

sfc-gh-tzhang left a comment

		public class FlushServiceTest {

		@Parameterized.Parameters(name = "isIcebergMode: {0}")

SNOW-1437885 Disable blob interleaving when in Iceberg mode #763

SNOW-1437885 Disable blob interleaving when in Iceberg mode #763

Conversation

sfc-gh-alhuang commented May 23, 2024

sfc-gh-gdoci May 24, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-hmadan left a comment

Choose a reason for hiding this comment

sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-alhuang commented May 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sfc-gh-hmadan left a 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-alhuang commented Jun 10, 2024

sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

sfc-gh-gdoci May 24, 2024 •

edited

Loading

sfc-gh-alhuang commented May 28, 2024 •

edited

Loading