SNOW-826772 Unification of Snowflake role and user for Snowpipe Streaming #650

sfc-gh-japatel · 2023-06-13T22:33:25Z

Here is what the connection object is used for: [Only for Snowpipe Streaming]
- DB existence checker.
- Schema existence checker.
- CreateTable
- Alter table

Do you need test for this change?

Existing tests uses a user and a uses a default role. [Check profile.json]
- Verifying existing tests passes is enough.
- ConnectionServiceIT is one test which verifies this code path.
It's not worth adding the another profile.json and modifying the existing tests to use a new user/role.
Manual tests: Ran a manual test in sfctest0 account with a new role and a new user. Also ran a test which revoked an insert privilege from this role and it resulted into channel open error.

Can anything go wrong with this change?

Yes, if the default role of the user has more privileges than the role supplied. The role being used for Snowpipe Streaming needs atleast an insert privilege. But most of the times, an insert priv is accompanied by usage priv as well.
- We will be adding this change in a new 2.0.0 version of Kafka Connector, so this will be advertised/documented.
- Only affects Snowpipe Streaming.

codecov · 2023-06-13T23:15:12Z

Codecov Report

Merging #650 (6164e89) into master (f8da3c5) will decrease coverage by 0.02%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master     #650      +/-   ##
==========================================
- Coverage   88.02%   88.01%   -0.02%     
==========================================
  Files          50       50              
  Lines        4124     4137      +13     
  Branches      445      448       +3     
==========================================
+ Hits         3630     3641      +11     
- Misses        327      329       +2     
  Partials      167      167

Impacted Files	Coverage Δ
...wflake/kafka/connector/internal/InternalUtils.java	`92.30% <71.42%> (-1.33%)`	⬇️
...or/internal/SnowflakeConnectionServiceFactory.java	`96.66% <100.00%> (+0.11%)`	⬆️

... and 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

sfc-gh-tzhang

lgtm!

sfc-gh-tzhang · 2023-06-15T20:16:52Z

src/main/java/com/snowflake/kafka/connector/internal/InternalUtils.java

+    if (ingestionMethodConfig == IngestionMethodConfig.SNOWPIPE_STREAMING) {
+      final String providedSFRoleInConfig = conf.get(Utils.SF_ROLE);
+      if (!Strings.isNullOrEmpty(providedSFRoleInConfig)) {
+        LOGGER.debug("Using provided role {} for JDBC connection.", providedSFRoleInConfig);


Could we combine the log here with the one in the else branch?

And I suggest to use INFO since DEBUG logging at this point is useless due to the large number of debug logging in the ingest SDK

Yes, makes sense. no harm in using info.

src/main/java/com/snowflake/kafka/connector/internal/InternalUtils.java

sfc-gh-rcheng · 2023-06-15T23:18:12Z

src/main/java/com/snowflake/kafka/connector/internal/InternalUtils.java

+   * @param sslEnabled is sslEnabled?
+   * @return Properties object which will be passed down to JDBC connection
+   */
+  static Properties createProperties(Map<String, String> conf, boolean sslEnabled) {


can we add snowpipe something to this method name (createPropertiesForSnowpipe) or mention in the comments its for snowpipe?

I didnt want make number of changes in test classes to reflect this change. I can add comment about using default ingestmethodconfig

…ming (snowflakedb#650)

sfc-gh-japatel added 2 commits May 31, 2023 13:31

Add role

4f505fb

Google jfmt

82ff182

sfc-gh-japatel requested review from sfc-gh-tzhang, sfc-gh-tjones, sfc-gh-rcheng and a team as code owners June 13, 2023 22:33

sfc-gh-tzhang approved these changes Jun 15, 2023

View reviewed changes

sfc-gh-asen reviewed Jun 15, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/internal/InternalUtils.java Outdated Show resolved Hide resolved

sfc-gh-xhuang reviewed Jun 15, 2023

View reviewed changes

src/main/java/com/snowflake/kafka/connector/internal/InternalUtils.java Outdated Show resolved Hide resolved

sfc-gh-japatel added 2 commits June 15, 2023 14:06

Minor nit comments resolved

9fa5191

NO-SNOW Kafka connector supports any encoding, not only UTF-8

25a8f53

sfc-gh-rcheng approved these changes Jun 15, 2023

View reviewed changes

Better javadoc

6164e89

sfc-gh-japatel merged commit f3a9fb6 into master Jun 16, 2023

sfc-gh-japatel deleted the japatel-role-unification branch June 16, 2023 19:47

khsoneji pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Oct 12, 2023

SNOW-826772 Unification of Snowflake role and user for Snowpipe Strea…

b03a034

…ming (snowflakedb#650)

khsoneji pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Oct 12, 2023

SNOW-826772 Unification of Snowflake role and user for Snowpipe Strea…

466bb36

…ming (snowflakedb#650)

khsoneji pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Oct 12, 2023

SNOW-826772 Unification of Snowflake role and user for Snowpipe Strea…

50fdc96

…ming (snowflakedb#650)

khsoneji pushed a commit to confluentinc/snowflake-kafka-connector that referenced this pull request Oct 12, 2023

SNOW-826772 Unification of Snowflake role and user for Snowpipe Strea…

d4a9d22

…ming (snowflakedb#650)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SNOW-826772 Unification of Snowflake role and user for Snowpipe Streaming #650

SNOW-826772 Unification of Snowflake role and user for Snowpipe Streaming #650

sfc-gh-japatel commented Jun 13, 2023 •

edited

Loading

codecov bot commented Jun 13, 2023 •

edited

Loading

sfc-gh-tzhang left a comment

sfc-gh-tzhang Jun 15, 2023

sfc-gh-tzhang Jun 15, 2023

sfc-gh-japatel Jun 15, 2023

sfc-gh-rcheng Jun 15, 2023

sfc-gh-japatel Jun 16, 2023

SNOW-826772 Unification of Snowflake role and user for Snowpipe Streaming #650

SNOW-826772 Unification of Snowflake role and user for Snowpipe Streaming #650

Conversation

sfc-gh-japatel commented Jun 13, 2023 • edited Loading

codecov bot commented Jun 13, 2023 • edited Loading

Codecov Report

sfc-gh-tzhang left a comment

Choose a reason for hiding this comment

sfc-gh-tzhang Jun 15, 2023

Choose a reason for hiding this comment

sfc-gh-tzhang Jun 15, 2023

Choose a reason for hiding this comment

sfc-gh-japatel Jun 15, 2023

Choose a reason for hiding this comment

sfc-gh-rcheng Jun 15, 2023

Choose a reason for hiding this comment

sfc-gh-japatel Jun 16, 2023

Choose a reason for hiding this comment

sfc-gh-japatel commented Jun 13, 2023 •

edited

Loading

codecov bot commented Jun 13, 2023 •

edited

Loading