Snow 1944643 network errors retry #1015
Conversation
| @@ -18,7 +18,7 @@ export DRIVER_NAME=nodejs | |||
| BUILD_IMAGE_VERSION=1 | |||
|
|
|||
| # Test Images | |||
| TEST_IMAGE_VERSION=1 | |||
| TEST_IMAGE_VERSION=2 | |||
There was a problem hiding this comment.
Do we really need this? We can update existing image and Jenkins always downloads fresh one.
| RUN pip install -U snowflake-connector-python | ||
|
|
||
| ENV JAVA_HOME=/usr/lib/jvm/java-17-openjdk | ||
| ENV PATH="$PATH:/usr/lib/jvm/java-17-openjdk/bin" |
There was a problem hiding this comment.
If you install via APK it is not automatically updated?
| @@ -468,6 +468,10 @@ exports.isRetryableHttpError = function (response, retry403) { | |||
| (response.statusCode === 429)); | |||
| }; | |||
|
|
|||
| exports.isNetworkError = function (err) { | |||
| return err.code === Errors.codes.ERR_SF_NETWORK_COULD_NOT_CONNECT; | |||
There was a problem hiding this comment.
Does it mean only when we can't connect? Is it the customer problem or maybe it was more like resets?
| await wireMock.global.shutdown(); | ||
| }); | ||
| it('Test retries - Connection reset', async function () { | ||
| // snowflake.configure({ |
| // logLevel: "DEBUG", | ||
| // disableOCSPChecks: true | ||
| // }); | ||
| await addWireMockMappingsFromFile(wireMock, 'wiremock/mappings/six_reset_connection_and_correct_response.json'); |
There was a problem hiding this comment.
Based only on mapping file name - should we also have a test, when it eventually ends up in connection reset propagated to the user?
| const os = require('os'); | ||
| const testUtil = require('../testUtil'); | ||
|
|
||
| if (os.platform !== 'win32') { |
There was a problem hiding this comment.
Is it a test for wiremock itself? Do we still need this?
|
|
||
| const timeout = new Promise((resolve, reject) => | ||
| timeoutHandle = setTimeout( | ||
| () => reject('Wiremock unavailable after 60s.'), |
There was a problem hiding this comment.
Wiremock gets up in a second, maybe we can lower this limit?
| } | ||
| }, | ||
| { | ||
| "scenarioName": "Heartbeat", |
There was a problem hiding this comment.
Maybe worth adding it by default to wiremock instead of adding it in each test?
| @@ -0,0 +1,34 @@ | |||
| { | |||
| name: PARAM_RETRY_SF_MAX_NUM_RETRIES, | ||
| defaultValue: 1000, | ||
| external: true, | ||
| validate: isNonNegativeInteger | ||
| }, | ||
| { | ||
| name: PARAM_RETRY_SF_STARTING_SLEEP_TIME, | ||
| defaultValue: 1, | ||
| external: true, | ||
| validate: isNonNegativeNumber | ||
| }, | ||
| { | ||
| name: PARAM_RETRY_SF_MAX_SLEEP_TIME, | ||
| external: true, |
There was a problem hiding this comment.
since now PARAM_RETRY_SF_MAX_NUM_RETRIES , PARAM_RETRY_SF_STARTING_SLEEP_TIME, and PARAM_RETRY_SF_MAX_SLEEP_TIME will be public, my respectful request here would be that please make sure the parameters are correctly documented in Snowflake docs. Thank you in advance! 🙇
This reverts commit 840b23d.
sfc-gh-pmotacki
force-pushed
the
SNOW-1944643-network-errors-retry
branch
from
849333f to
5df04d5
Compare
Description
Please explain the changes you made here.
Checklist
npm run lint:check -- CHANGED_FILESand fix problems in changed code)npm run test:unitandnpm run test:integration)