Azurite icon indicating copy to clipboard operation
Azurite copied to clipboard

add blobKeepAliveTimeout, queueKeepAliveTimeout, tableKeepAliveTimeout

Open Slach opened this issue 1 year ago • 17 comments

second try after https://github.com/Azure/Azurite/pull/2443

fix #2053 and workaround ClickHouse/ClickHouse#60447

Slach avatar Aug 26 '24 16:08 Slach

@Slach

Would you please share more background of this change?

  1. Does it target for a temp fix of CPP SDK issue
  2. 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 https://github.com/Azure/Azurite/pull/2443#issuecomment-2268057651, we will take the change to public Azurite, you can build your own private Azurite as a workaround.

blueww avatar Aug 27 '24 05:08 blueww

  1. 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

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

Slach avatar Aug 27 '24 06:08 Slach

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

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

Slach avatar Aug 27 '24 06:08 Slach

  1. 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
  2. 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.

blueww avatar Aug 27 '24 07:08 blueww

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 https://github.com/nodejs/node/issues/34560

Slach avatar Aug 27 '24 07:08 Slach

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?

blueww avatar Aug 27 '24 07:08 blueww

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.

Slach avatar Aug 28 '24 12:08 Slach

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

blueww avatar Aug 29 '24 05:08 blueww

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 https://github.com/nodejs/node/issues/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

Slach avatar Aug 29 '24 08:08 Slach

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.

blueww avatar Sep 02 '24 09:09 blueww

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

Slach avatar Sep 02 '24 09:09 Slach

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

Slach avatar Sep 03 '24 09:09 Slach

@blueww could you trigger github actions?

Slach avatar Sep 03 '24 17:09 Slach

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

blueww avatar Sep 09 '24 06:09 blueww

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

blueww avatar Sep 18 '24 08:09 blueww

sorry i'm on vacation will fix when return

Slach avatar Sep 18 '24 16:09 Slach

Hi @blueww, i prepared some changes need your advice about tests https://github.com/Azure/Azurite/pull/2454#discussion_r1798951345

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

Slach avatar Oct 14 '24 08:10 Slach

@blueww could you trigger CI/CD? Hope PR ready for merge

Slach avatar Nov 20 '24 08:11 Slach

@blueww Node 16.x doesn't contain keep-alive: timeout=5 header, please trigger CI/CD again

Slach avatar Nov 20 '24 11:11 Slach

@XiaoningLiu, @EmmaZhux any news from your side?

Slach avatar Nov 22 '24 07:11 Slach

@XiaoningLiu, @EmmaZhu could you merge this PR?

Slach avatar Nov 25 '24 05:11 Slach