add blobKeepAliveTimeout, queueKeepAliveTimeout, tableKeepAliveTimeout #2454

Slach · 2024-08-26T16:45:04Z

second try after #2443

fix #2053 and workaround ClickHouse/ClickHouse#60447

blueww · 2024-08-27T05:18:39Z

Would you please share more background of this change?

Does it target for a temp fix of CPP SDK issue
Does product Azure server has same behavior after the 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.

Slach · 2024-08-27T06:30:19Z

This change allows properly using azurite in e2e test scenarios with old version of azure cpp-sdk
current version of cpp sdk still contains issue when client dont respect Keep-alive header from server
Yes, by default azurite will have the same behavior as with the previous version, when send Keep-alive: timeout=5

Slach · 2024-08-27T06:31:24Z

You can't upgrade azure cpp sdk version in already released products

too much work for backports even when new cpp sdk will release

blueww · 2024-08-27T07:21:40Z

This change allows properly using azurite in e2e test scenarios with old version of azure cpp-sdk
current version of cpp sdk still contains issue when client dont respect Keep-alive header from server

Yes, by default azurite will have the same behavior as with the previous version, when send Keep-alive: timeout=5

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.

Slach · 2024-08-27T07:44:51Z

If this is only for a temp SDK issue fix

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

blueww · 2024-08-27T07:55:20Z

If this is only for a temp SDK issue fix

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?
And if user set none default value, will Azurite behavior different from product Azure?

Slach · 2024-08-28T12:47:31Z

for second point, if user set none default value, will Azurite behavior different from product Azure?

when you pass --blobKeepAliveTimeout=0 or --blobKeepAliveTimeout=""
then this parameter will just ignore and nodejs httpServer will initialize with default 5000ms value

Actually, Azurite is different with Azure in this part, right now, without this PR

Azurite already send Keep-alive: timeout=5 header in server response
but Azure doesn't

check how exactly nodejs handle Keep-Alive

Azure example request+response

   PUT https://xxxbackups.blob.core.windows.net/xxxbackups/altinity-cloud-managed-clickhouse/backup-test/full_backup_8596655552399465575/shadow/test_partitions_TestIntegrationAzure/t2/default_435ef8e8df48e77cefdcc330dc8b8d8c_3_3_0.tar?blockid=lPNYkFCYSziFsOQF4bwpJQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA%3D%3D&comp=block&timeout=14401
   Authorization: REDACTED
   Content-Length: [12288]
   User-Agent: [Azure-Storage/0.15 (go1.22.6; linux)]
   X-Ms-Client-Request-Id: [6cfaab65-8c46-4f48-68b5-a992ec7524ee]
   X-Ms-Version: [2020-10-02]
   x-ms-date: [Wed, 28 Aug 2024 12:31:05 GMT]
   --------------------------------------------------------------------------------
   RESPONSE Status: 201 Created
   Content-Length: [0]
   Date: [Wed, 28 Aug 2024 12:29:30 GMT]
   Server: [Windows-Azure-Blob/1.0 Microsoft-HTTPAPI/2.0]
   X-Ms-Client-Request-Id: [6cfaab65-8c46-4f48-68b5-a992ec7524ee]
   X-Ms-Content-Crc64: [ppWapnj/W7U=]
   X-Ms-Request-Id: [9b01ff55-d01e-006f-0445-f946cc000000]
   X-Ms-Request-Server-Encrypted: [true]
   X-Ms-Version: [2020-10-02]

Azurite for the same code

PUT http://devstoreaccount1.blob.azure:10000/container1/backup/full_backup_2226074798298314988/shadow/test_partitions_TestIntegrationAzure/t1/default_0%252D20220102_2_2_0.tar?blockid=sCJz0hyuTlWyUK7qdIfDHgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA%3D%3D&comp=block&timeout=14401
   Authorization: REDACTED
   Content-Length: [12288]
   User-Agent: [Azure-Storage/0.15 (go1.22.6; linux)]
   X-Ms-Client-Request-Id: [161470f4-dc6f-41e4-6ea7-6f2e39b7ad9d]
   X-Ms-Version: [2020-10-02]
   x-ms-date: [Wed, 28 Aug 2024 12:45:10 GMT]
   --------------------------------------------------------------------------------
   RESPONSE Status: 201 Created
   Connection: [keep-alive]
   Content-Length: [0]
   Date: [Wed, 28 Aug 2024 12:45:10 GMT]
   Keep-Alive: [timeout=5]
   Server: [Azurite-Blob/3.32.0]
   X-Ms-Client-Request-Id: [161470f4-dc6f-41e4-6ea7-6f2e39b7ad9d]
   X-Ms-Request-Id: [bd712af4-70f8-4c94-aecd-af01199f6399]
   X-Ms-Request-Server-Encrypted: [true]
   X-Ms-Version: [2024-11-04]

Connection: [keep-alive]
Keep-Alive: [timeout=5]

present in Azurite response headers.

blueww · 2024-08-29T05:04:49Z

@Slach

Thanks for the details!

I see Azurite will return 2 more headers than Azure server :

Connection: keep-alive
Keep-Alive: timeout=5

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?
And could we just remove the 2 headers instead adding the customized keeplive time?

We will review/test the PR and update you later.

src/queue/QueueServer.ts

Slach · 2024-08-29T08:13:24Z

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?

could you read details in nodejs/node#34560 ?

this is nodejs standard library behavior for any nodejs application which use httpServer, unfortunately i not so familiar with nodejs to make PR to nodejs

need time to think how to do it, current PR just allow us avoid conflicts with cpp-sdk

README.md

blueww · 2024-09-02T09:03:38Z

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?

could you read details in nodejs/node#34560 ?

this is nodejs standard library behavior for any nodejs application which use httpServer, unfortunately i not so familiar with nodejs to make PR to nodejs

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.
If really can't find a way to get so, keep the default as current behavior should be the choice.

Slach · 2024-09-02T09:06:03Z

If really can't find a way to get so, keep the default as current behavior should be the choice.

This is my working plan

Azure#2454 (comment) and Azure#2454 (comment)

Slach · 2024-09-03T09:38:23Z

Unfortunately, I didn't find how to completely disable Keep-Alive: timeout=XX header, will try to continue

Slach · 2024-09-03T17:28:34Z

@blueww could you trigger github actions?

blueww · 2024-09-09T06:15:13Z

README.md

@@ -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 5000


Could more description add here? Like what's the unit of the time, is it ms or s? And the impact by adding this parameter.

After discuss with my team member, a question is raised:
When set blobKeepAliveTimeout = 5001, will the responds still be Keep-Alive: [timeout=5] or Keep-Alive: [timeout=5.001] . If the before one, we might should only allow user to set timeout with unit as second here, the default value should be 5.

I figure out and switched from ms to seconds, look 11704a3

blueww · 2024-09-09T06:16:43Z

tests/BlobTestServerFactory.ts

@@ -4,6 +4,7 @@ import SqlBlobConfiguration from "../src/blob/SqlBlobConfiguration";
 import SqlBlobServer from "../src/blob/SqlBlobServer";
 import { StoreDestinationArray } from "../src/common/persistence/IExtentStore";
 import { DEFAULT_SQL_OPTIONS } from "../src/common/utils/constants";
+import { DEFAULT_BLOB_KEEP_ALIVE_TIMEOUT } from "../src/blob/utils/constants";


Could we add test case for B/Q/T service to check adding this parameter works., to avoid any regression in the future break the function?

Sorry for late response, could you suggest how to make this TestCase? How to check response headers to Keepalive-Timeout header exists and contains properly value?

@EmmaZhu

Could you please share how to get a header in raw server responds with JS SDK?

@EmmaZhu any news from your side?

Hi @Slach ,

You can use code like following to get keep-alive header:

const properties = await newBlobClient.getProperties(); const keepAliveHeader = properties._response.headers.get("keep-alive");

blueww · 2024-09-09T06:18:24Z

@Slach
I believe the PR is ready to review after you update it.
Just add 2 comments for the parameter description and test cases.

README.mcr.md

blueww · 2024-09-18T08:44:45Z

@Slach

Could you please look at my comments above, and see if can fix them.
Feel free to let me know if you have different opinion.

Slach · 2024-09-18T16:59:22Z

sorry i'm on vacation will fix when return

…t for fix Azure#2053 and workaround ClickHouse/ClickHouse#60447

…KeepAliveTimeout

Azure#2454 (comment) and Azure#2454 (comment)

…t for fix Azure#2053 and workaround ClickHouse/ClickHouse#60447 Signed-off-by: Slach <[email protected]>

… value

…m ms to seconds after review https://github.com/Azure/Azurite/pull/2454/files#r1751590590

…#r1751590590

Slach · 2024-10-14T08:45:23Z

Hi @blueww, i prepared some changes need your advice about tests
#2454 (comment)

current test code base is too complicated for my understanding
i tested changes internally manually and it works

blueww · 2024-10-22T07:42:35Z

src/common/VSCServerManagerQueue.ts

@@ -77,6 +77,7 @@ export default class VSCServerManagerBlob extends VSCServerManagerBase {
    const config = new QueueConfiguration(
      env.queueHost(),
      env.queuePort(),
+      env.blobKeepAliveTimeout(),


should be env.queueKeepAliveTimeout()?

Slach added a commit to Altinity/clickhouse-backup that referenced this pull request Aug 26, 2024

blobKeepAliveTimeout 600000, wait review Azure/Azurite#2454

d07a247

blueww reviewed Aug 29, 2024

View reviewed changes

src/queue/QueueServer.ts Outdated Show resolved Hide resolved

blueww reviewed Sep 2, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

blueww reviewed Sep 2, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

blueww closed this Sep 2, 2024

blueww reopened this Sep 2, 2024

Slach added a commit to Slach/Azurite that referenced this pull request Sep 3, 2024

actualize *keepAliveTimeout description and default values after review

c66120e

Azure#2454 (comment) and Azure#2454 (comment)

Slach added a commit to Slach/Azurite that referenced this pull request Sep 3, 2024

actualize *keepAliveTimeout description and default values after review

5a147d5

Azure#2454 (comment) and Azure#2454 (comment)

blueww reviewed Sep 9, 2024

View reviewed changes

blueww reviewed Sep 10, 2024

View reviewed changes

README.mcr.md Show resolved Hide resolved

Slach added 3 commits October 14, 2024 12:17

add blobKeepAliveTimeout, queueKeepAliveTimeout, tableKeepAliveTimeou…

96b03e9

…t for fix Azure#2053 and workaround ClickHouse/ClickHouse#60447

ChangeLog.md added blobKeepAliveTimeout, queueKeepAliveTimeout, table…

ce7f2fa

…KeepAliveTimeout

format constant imports

d303a8f

Slach added 3 commits October 14, 2024 12:17

actualize *keepAliveTimeout description and default values after review

8ee5221

Azure#2454 (comment) and Azure#2454 (comment)

add blobKeepAliveTimeout, queueKeepAliveTimeout, tableKeepAliveTimeou…

fe72724

…t for fix Azure#2053 and workaround ClickHouse/ClickHouse#60447 Signed-off-by: Slach <[email protected]>

increase DEFAULT_*_KEEP_ALIVE_TIMEOUT=5000, it was HttpServer default…

38518ce

… value

Slach force-pushed the main branch from 5a147d5 to 38518ce Compare October 14, 2024 07:37

Slach added 2 commits October 14, 2024 13:07

actualize *keepAliveTimeout description and switch default values fro…

11704a3

…m ms to seconds after review https://github.com/Azure/Azurite/pull/2454/files#r1751590590

actualize README for https://github.com/Azure/Azurite/pull/2454/files…

4c8aeaa

…#r1751590590

blueww reviewed Oct 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add blobKeepAliveTimeout, queueKeepAliveTimeout, tableKeepAliveTimeout #2454

add blobKeepAliveTimeout, queueKeepAliveTimeout, tableKeepAliveTimeout #2454

Slach commented Aug 26, 2024

blueww commented Aug 27, 2024

Slach commented Aug 27, 2024

Slach commented Aug 27, 2024

blueww commented Aug 27, 2024 •

edited

Loading

Slach commented Aug 27, 2024 •

edited

Loading

blueww commented Aug 27, 2024

Slach commented Aug 28, 2024 •

edited

Loading

blueww commented Aug 29, 2024 •

edited

Loading

Slach commented Aug 29, 2024 •

edited

Loading

blueww commented Sep 2, 2024

Slach commented Sep 2, 2024

Slach commented Sep 3, 2024

Slach commented Sep 3, 2024

blueww Sep 9, 2024 •

edited

Loading

Slach Oct 14, 2024

blueww Sep 9, 2024 •

edited

Loading

Slach Oct 14, 2024

blueww Oct 15, 2024

Slach Oct 18, 2024

EmmaZhu Oct 21, 2024

blueww commented Sep 9, 2024 •

edited

Loading

blueww commented Sep 18, 2024

Slach commented Sep 18, 2024

Slach commented Oct 14, 2024

blueww Oct 22, 2024 •

edited

Loading

add blobKeepAliveTimeout, queueKeepAliveTimeout, tableKeepAliveTimeout #2454

Are you sure you want to change the base?

add blobKeepAliveTimeout, queueKeepAliveTimeout, tableKeepAliveTimeout #2454

Conversation

Slach commented Aug 26, 2024

blueww commented Aug 27, 2024

Slach commented Aug 27, 2024

Slach commented Aug 27, 2024

blueww commented Aug 27, 2024 • edited Loading

Slach commented Aug 27, 2024 • edited Loading

blueww commented Aug 27, 2024

Slach commented Aug 28, 2024 • edited Loading

blueww commented Aug 29, 2024 • edited Loading

Slach commented Aug 29, 2024 • edited Loading

blueww commented Sep 2, 2024

Slach commented Sep 2, 2024

Slach commented Sep 3, 2024

Slach commented Sep 3, 2024

blueww Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Slach Oct 14, 2024

Choose a reason for hiding this comment

blueww Sep 9, 2024 • edited Loading

Choose a reason for hiding this comment

Slach Oct 14, 2024

Choose a reason for hiding this comment

blueww Oct 15, 2024

Choose a reason for hiding this comment

Slach Oct 18, 2024

Choose a reason for hiding this comment

EmmaZhu Oct 21, 2024

Choose a reason for hiding this comment

blueww commented Sep 9, 2024 • edited Loading

blueww commented Sep 18, 2024

Slach commented Sep 18, 2024

Slach commented Oct 14, 2024

blueww Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

blueww commented Aug 27, 2024 •

edited

Loading

Slach commented Aug 27, 2024 •

edited

Loading

Slach commented Aug 28, 2024 •

edited

Loading

blueww commented Aug 29, 2024 •

edited

Loading

Slach commented Aug 29, 2024 •

edited

Loading

blueww Sep 9, 2024 •

edited

Loading

blueww Sep 9, 2024 •

edited

Loading

blueww commented Sep 9, 2024 •

edited

Loading

blueww Oct 22, 2024 •

edited

Loading