-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add blobKeepAliveTimeout, queueKeepAliveTimeout, tableKeepAliveTimeout #2443
Conversation
@microsoft-github-policy-service agree |
/azp run |
Azure Pipelines could not run because the pipeline triggers exclude this branch/path. |
Thanks for the contribution! And could you give more details on why the change can fix the issue? |
rebased |
PR description updated |
Besides test class "FSExtentStore" failure on BlobTest_Win_Node* is know issue which is in fixing, other test failures are new and related with this PR change. Most of the new failure looks like :
|
@@ -186,10 +186,13 @@ Following extension configurations are supported: | |||
|
|||
- `azurite.blobHost` Blob service listening endpoint, by default 127.0.0.1 | |||
- `azurite.blobPort` Blob service listening port, by default 10000 | |||
- `azurite.blobKeepAliveTimeout` Blob service keep alive timeout, by default 5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the value before this PR?
Could we use the value before this PR is merged to avoid regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was 5 by default
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks might not be 5 for all scenarios, since so many test cases failed with "read ECONNRESET", looks the network break since keep live timeout.
sorry how to match which test exactly doesn't work? And how to run only this failed test? i tried to run and got lot of failures can't figure out how to debug it |
This RP looks target for a temp fix of CPP SDK issue (you said : "We are going to fix this behavior in the next version of clickhouse-server"), and this looks a high risk change since there are so many test cases failed. I would suggest you contact CPP SDK team if this is a CPP SDK issue. |
Close this PR as its looks a temp fix for client SDK issue, and take many Azurite Unit test fail. |
@blueww |
fix #2053
workaround for ClickHouse/ClickHouse#60447
we use azurite for Azure BLOB storage in our integration tests
Azure CPP Sdk (or it usage on clickhouse-server side) don't respect
Keep-Alive: 5
header which sent from azurite side, and continue to make request to already closed connection after 5 seconds, this header sent from standard nodejs http libraryWe are going to fix this behavior in the next version of
clickhouse-server
but currently to allow pass e2e tests, we would like to propose adding cli option and config settings to allow increase value for Keep-Alive header
we tested these changes and it actually works, hope your CI/CD pipelines also will pass