Snow 1853185 jdbc driver v3.16 native okta http retry storm #2064

sfc-gh-fpawlowski · 2025-02-05T13:32:40Z

Overview

SNOW-1853185

Pre-review self checklist

PR branch is updated with all the changes from master branch
The code is correctly formatted (run mvn -P check-style validate)
New public API is not unnecessary exposed (run mvn verify and inspect target/japicmp/japicmp.html)
The pull request name is prefixed with SNOW-XXXX:
Code is in compliance with internal logging requirements

External contributors - please answer these questions before submitting a pull request. Thanks!

What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.

Issue: #NNNN
Fill out the following pre-review checklist:
- I am adding a new automated test(s) to verify correctness of my new code
- I am adding new logging messages
- I am modifying authorization mechanisms
- I am adding new credentials
- I am modifying OCSP code
- I am adding a new dependency or upgrading an existing one
- I am adding new public/protected component not marked with @SnowflakeJdbcInternalApi (note that public/protected methods/fields in classes marked with this annotation are already internal)
Please describe how your code solves the related issue.

Please write a short description of how your code change solves the related issue.

… into SNOW-1853185-JDBC-Driver-v3.16-Native-Okta-HTTP-Retry-Storm

…ative-Okta-HTTP-Retry-Storm' into SNOW-1853185-JDBC-Driver-v3.16-Native-Okta-HTTP-Retry-Storm

…-HTTP-Retry-Storm

…ling jobs

sfc-gh-fpawlowski · 2025-02-10T00:55:27Z

src/main/java/net/snowflake/client/jdbc/RestRequest.java

@@ -503,6 +527,9 @@ public static CloseableHttpResponse execute(
                requestIdStr,
                requestInfoScrubbed,
                backoffInMilli);
+            //            TODO: shouldn't we sleep here for backoffInMilli - elapsedMilliForLastCall
+            // ?
+            //            Thread.sleep(backoffInMilli - elapsedMilliForLastCall);
            Thread.sleep(backoffInMilli);


Shouldn't we sleep here for backoffInMilli - elapsedMilliForLastCall?

src/main/java/net/snowflake/client/CallableThrowingSnowflakeSqlException.java

TestOnly/pom.xml

src/main/java/net/snowflake/client/CallableThrowingSnowflakeSqlException.java

sfc-gh-dprzybysz · 2025-02-10T11:53:25Z

src/main/java/net/snowflake/client/core/HttpUtil.java

@@ -621,6 +622,24 @@ static String executeRequestWithoutCookies(
        new ExecTimeTelemetryData());
  }

+  public static String executeGeneralRequest(


let's mark as internal or remove public

I'm not sure if that's possible - it is still used in multiple places - e.g. in SessionUtil.java
(I gave a longer answer in the next comment to why I kept those)

snowflake-jdbc/src/main/java/net/snowflake/client/core/SessionUtil.java

Lines 1063 to 1069 in 6d5d616

HttpUtil.executeGeneralRequest(

postRequest,

loginInput.getLoginTimeout(),

loginInput.getAuthTimeout(),

loginInput.getSocketTimeoutInMillis(),

0,

loginInput.getHttpClientSettingsKey());

then we can mark some methods with @SnowflakeJdbcInternalApi

sfc-gh-dprzybysz · 2025-02-10T12:24:52Z

src/main/java/net/snowflake/client/util/RetryContextManager.java

+   *
+   * @return the retry hook.
+   */
+  public RetryHook getRetryHook() {


do we need to extract the hook? cannot the hook be called indirectly via some method on RetryContextManager?

I considered the RetryHook Enum as a part of the RetryContext 'interface' - we can specify it in constructor, and adjust outer behavior based on its value later (i.e. when to execute the passed retry logic).

We could consider defining subclasses of the RetryContext class - each representing different type of retry behavior (represented originally by the RetryHook enumeration).
It would be also possible to extract the RetryHook-related data with boolean methods (e.g. isAlwaysRetry() ...) - but I don't think this approach has a significant advantages over the original one)

src/test/java/net/snowflake/client/core/SessionUtilWiremockIT.java

sfc-gh-dprzybysz · 2025-02-10T12:27:35Z

src/test/java/net/snowflake/client/core/SessionUtilWiremockIT.java

+              + vanityUrlCalls.size());
+    }
+
+    // Ensure each consecutive pair of calls has at least 1-second gap (1000 ms).


is the one second gap configurable? if so let's make it shorter for tests

The 1-second gap comes from the minimum length of the backoff during retries in RestRequest. It is not configurable from what I understand.

snowflake-jdbc/src/main/java/net/snowflake/client/jdbc/RestRequest.java

Line 51 in 6d5d616

private static final long minBackoffInMilli = 1000;

src/test/java/net/snowflake/client/core/SessionUtilWiremockIT.java

sfc-gh-dprzybysz · 2025-02-10T12:33:33Z

src/test/java/net/snowflake/client/jdbc/BaseWiremockTest.java

+
+    // If not found, attempt to read from the classpath (resources directory).
+    // Has to start from '/' followed by a subdirectory of the resources directory.
+    try (InputStream in = getClass().getResourceAsStream(filePath)) {


can be simpler?

snowflake-jdbc/src/test/java/net/snowflake/client/jdbc/StreamLatestIT.java

Lines 284 to 289 in 2355c5a

try (InputStream inputStream =

conn.unwrap(SnowflakeConnectionV1.class)

.downloadStream(stageName, fileName, decompress);

InputStreamReader isr = new InputStreamReader(inputStream);

BufferedReader br = new BufferedReader(isr)) {

String content = br.lines().collect(Collectors.joining("\n"));

Good idea - reimplemented this way 👍

…-HTTP-Retry-Storm

…ments handling

src/main/java/net/snowflake/client/util/RetryContextManager.java

sfc-gh-fpawlowski added 8 commits February 4, 2025 12:55

SNOW-1853185: Base for OktaWiremockIT

a657244

SNOW-1853185: OktaWiremockIT with working endpoint

7cc2b33

SNOW-1853185: OktaWiremockIT with working endpoint

23fd381

SNOW-1853185: OktaWiremockIT with port inclusion comments

6ae4b1f

Merge branch 'SNOW-1853185-jdbc-driver-v-3-16-native-okta-http-retry'…

7f38b4a

… into SNOW-1853185-JDBC-Driver-v3.16-Native-Okta-HTTP-Retry-Storm

Update SnowflakeDriverIT.java

fcc5e08

Merge remote-tracking branch 'origin/SNOW-1853185-JDBC-Driver-v3.16-N…

adf022c

…ative-Okta-HTTP-Retry-Storm' into SNOW-1853185-JDBC-Driver-v3.16-Native-Okta-HTTP-Retry-Storm

SNOW-1853185: Reverted test commit

110faf0

sfc-gh-fpawlowski self-assigned this Feb 5, 2025

sfc-gh-fpawlowski added 16 commits February 5, 2025 16:10

SNOW-1853185: Wiremock working test

0ab28ca

SNOW-1853185: Wiremock files structure organized

675a045

SNOW-1853185: Wiremock files read from file

f553892

SNOW-1853185: Rest request refactored

ffabd53

SNOW-1853185: Retry context manager added

4a54ab5

SNOW-1853185: Retry manager called

d393961

Merge branch 'master' into SNOW-1853185-JDBC-Driver-v3.16-Native-Okta…

4b60e61

…-HTTP-Retry-Storm

SNOW-1853185: Revert renaming isSocketTimeoutNoBackoff because of fai…

c6daa3d

…ling jobs

SNOW-1853185: Added missing dependencies to tests

5cb1272

SNOW-1853185: Javadoc fixes

c0c6389

SNOW-1853185: Javadoc fixes

e9e964d

SNOW-1853185: Formatting run

d049d2c

SNOW-1853185: Changed test to check time between calls to okta

73095ff

SNOW-1853185: Made tests check 429 is waited. Added deserialization

9327ce9

SNOW-1853185: Code formatted

d6dbcf5

SNOW-1853185: Code formatted

0423238

sfc-gh-fpawlowski commented Feb 10, 2025

View reviewed changes

sfc-gh-fpawlowski added 3 commits February 10, 2025 01:56

SNOW-1853185: TODOs removed

fddcf10

SNOW-1853185: Replaced .* imports with specific

2bae032

SNOW-1853185: Formatted

d05b8c6

sfc-gh-fpawlowski marked this pull request as ready for review February 10, 2025 01:07

sfc-gh-fpawlowski requested a review from a team as a code owner February 10, 2025 01:07

sfc-gh-pfus reviewed Feb 10, 2025

View reviewed changes

src/main/java/net/snowflake/client/CallableThrowingSnowflakeSqlException.java Outdated Show resolved Hide resolved

sfc-gh-dprzybysz requested changes Feb 10, 2025

View reviewed changes

sfc-gh-fpawlowski added 4 commits February 11, 2025 15:15

Merge branch 'master' into SNOW-1853185-JDBC-Driver-v3.16-Native-Okta…

1c66ff4

…-HTTP-Retry-Storm

SNOW-1853185: Replaced custom Exception type with ThrowingCallable

478a360

SNOW-1853185: Code cleanup - added docs, improved visibility and argu…

e3b8e2f

…ments handling

SNOW-1853185: Added javadocs and comments from PR

780f97a

sfc-gh-fpawlowski commented Feb 12, 2025

View reviewed changes

src/main/java/net/snowflake/client/util/RetryContextManager.java Outdated Show resolved Hide resolved

sfc-gh-fpawlowski added 8 commits February 12, 2025 15:38

SNOW-1853185: ThrowingFunction added and RetryManager migrated to use it

678970f

SNOW-1853185: Renamed RetryContextManager to RetryContext

0782b70

SNOW-1853185: visibility limited of retrycontext

f4506b3

SNOW-1853185: Changed assertion types to hamcrest

0d8df42

SNOW-1853185: Implemented reading files using BufferedReader

1e3c87b

SNOW-1853185: Test pom.xml - version moved to properties

f3b3d19

SNOW-1853185: Code formatted

80f8ceb

SNOW-1853185: Code formatted

1fa8914

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Snow 1853185 jdbc driver v3.16 native okta http retry storm #2064

Snow 1853185 jdbc driver v3.16 native okta http retry storm #2064

sfc-gh-fpawlowski commented Feb 5, 2025 •

edited

Loading

sfc-gh-fpawlowski Feb 10, 2025

sfc-gh-dprzybysz Feb 10, 2025

sfc-gh-fpawlowski Feb 11, 2025

sfc-gh-dprzybysz Feb 12, 2025

sfc-gh-dprzybysz Feb 10, 2025

sfc-gh-fpawlowski Feb 12, 2025

sfc-gh-dprzybysz Feb 10, 2025

sfc-gh-fpawlowski Feb 12, 2025

sfc-gh-dprzybysz Feb 10, 2025

sfc-gh-fpawlowski Feb 12, 2025

	HttpUtil.executeGeneralRequest(
	postRequest,
	loginInput.getLoginTimeout(),
	loginInput.getAuthTimeout(),
	loginInput.getSocketTimeoutInMillis(),
	0,
	loginInput.getHttpClientSettingsKey());

	try (InputStream inputStream =
	conn.unwrap(SnowflakeConnectionV1.class)
	.downloadStream(stageName, fileName, decompress);
	InputStreamReader isr = new InputStreamReader(inputStream);
	BufferedReader br = new BufferedReader(isr)) {
	String content = br.lines().collect(Collectors.joining("\n"));

Snow 1853185 jdbc driver v3.16 native okta http retry storm #2064

Are you sure you want to change the base?

Snow 1853185 jdbc driver v3.16 native okta http retry storm #2064

Conversation

sfc-gh-fpawlowski commented Feb 5, 2025 • edited Loading

Overview

Pre-review self checklist

External contributors - please answer these questions before submitting a pull request. Thanks!

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-fpawlowski commented Feb 5, 2025 •

edited

Loading