-
Notifications
You must be signed in to change notification settings - Fork 332
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 #2454
Conversation
Would you please share more background of this change?
If it target a temp fix, or will make Azurite behavior not aligned with product Azure, as I have indicated in the before PR #2443 (comment), we will take the change to public Azurite, you can build your own private Azurite as a workaround. |
|
You can't upgrade azure cpp sdk version in already released products too much work for backports even when new cpp sdk will release |
for #2, if user set none default value, will Azurite behavior different from product Azure? If this is only for a temp SDK issue fix, Azurite won't take it. Especially when the change could make Azurite behavior different from product Azure. We would suggest building your own private Azurite package. |
this is not temp SDK issue fix. You can't guarantee same behavior from other SDKs issue become to your product from standard nodejs nodejs/node#34560 |
Does the client SDK with this issue works with product Azure Server? |
when you pass --blobKeepAliveTimeout=0 or --blobKeepAliveTimeout="" Actually,
check how exactly nodejs handle Keep-Alive
Connection: [keep-alive] present in |
Thanks for the details! I see Azurite will return 2 more headers than Azure server :
In this case, it's better we make the default Azurite behavior aligned with Azure server as not return the 2 headers. Would you please help to update the PR for this? We will review/test the PR and update you later. |
could you read details in nodejs/node#34560 ? this is nodejs standard library behavior for any nodejs application which use need time to think how to do it, current PR just allow us avoid conflicts with cpp-sdk |
Hope we could make the default behavior works like Azure server which won't return the headers. |
This is my working plan |
Unfortunately, I didn't find how to completely disable |
@blueww could you trigger github actions? |
@Slach |
Could you please look at my comments above, and see if can fix them. |
sorry i'm on vacation will fix when return |
Hi @blueww, i prepared some changes need your advice about tests current test code base is too complicated for my understanding |
…t for fix Azure#2053 and workaround ClickHouse/ClickHouse#60447
…t for fix Azure#2053 and workaround ClickHouse/ClickHouse#60447 Signed-off-by: Slach <[email protected]>
@blueww could you trigger CI/CD? Hope PR ready for merge |
@blueww |
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.
@XiaoningLiu , @EmmaZhu
Would you please help to look ?
@XiaoningLiu, @EmmaZhux any news from your side? |
@XiaoningLiu, @EmmaZhu could you merge this PR? |
second try after #2443
fix #2053 and workaround ClickHouse/ClickHouse#60447