Conversation
* Port CH #59176 (not include readonly insert) * Port CH #57054: clean up settings usage * Port CH #59380: Cleanup connection pool surroundings * Port CH #59501: better initial replicas failover * Port CH #59534: Use ConnectionPoolPtr instead of raw pointer * Port CH #72666: parallel replicas update cluster's errors_count * Port CH #60158: fix custom key failover test flakyness * Port CH #50036: Load balancing bugfixes
* Port CH #66644: Fix data race in S3::ClientCache * Port CH #56475: recheck 404 response for complete multipart upload * Port CH #58343 and #55559 * Port CH #58962: Fix memory management in copyDataToS3File * Port CH #46451: Wait for tasks finish when scheduler failed * Port CH #66295: Fix error reporting while copying to S3 or AzureBlobStorage stash * Port CH #49718: Fix typo * Port CH #71090 & #79322: Less logs * Port CH #78185: Fix incorrect S3 uri parsing when keyis not specified on path style * Port CH #77678: Do not retry S3 requests in case of host not found * Port CH #64714: Check for missing Upload ID in CreateMultipartUpload reply
* Port CH #47486: Remove asynchronous connection drain * Port CH #42130: Fix possible timeout exception for distributed queries with use_hedged_requests=0 * Port CH #53050: Fix summary reporting with parallel replicas with LIMIT fix
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffbb8a82ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// TODO Absolutely wrong code: read_list could be empty; motivation of rand is unclear. | ||
| /// This code path is disabled by default. | ||
|
|
||
| auto & socket = read_list[thread_local_rng() % read_list.size()]; |
There was a problem hiding this comment.
In getReplicaForReading, only n == 0 is treated as a timeout now, so a select() error (n < 0, e.g., EBADF when a replica socket closes) falls through to read_list[...] without any readiness or emptiness check. If Poco clears read_list on error this becomes a modulo-by-zero crash; even if it doesn’t, you may read from a socket that wasn’t marked ready and block up to receive_timeout while the bad replica is never invalidated. Previously n <= 0 or read_list.empty() would invalidate and throw; you should restore error handling for n < 0 or guard read_list before indexing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffbb8a82ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// TODO Absolutely wrong code: read_list could be empty; motivation of rand is unclear. | ||
| /// This code path is disabled by default. | ||
|
|
||
| auto & socket = read_list[thread_local_rng() % read_list.size()]; |
There was a problem hiding this comment.
In getReplicaForReading, only n == 0 is treated as a timeout now, so a select() error (n < 0, e.g., EBADF when a replica socket closes) falls through to read_list[...] without any readiness or emptiness check. If Poco clears read_list on error this becomes a modulo-by-zero crash; even if it doesn’t, you may read from a socket that wasn’t marked ready and block up to receive_timeout while the bad replica is never invalidated. Previously n <= 0 or read_list.empty() would invalidate and throw; you should restore error handling for n < 0 or guard read_list before indexing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffbb8a82ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (size_t i = 1; i < key.size(); i++) | ||
| { | ||
| if (key[i - 1] == '/' && key[i] == '/') | ||
| { |
There was a problem hiding this comment.
The new validation rejects any key containing //. S3 object keys are opaque strings and may legally contain consecutive slashes (e.g., s3://bucket/a//b). With this check, those valid objects now throw BAD_ARGUMENTS even though the key is allowed and may already exist. This is a regression for users who encode empty path segments in their keys; the validation should avoid rejecting valid key contents.
Useful? React with 👍 / 👎.
unrelated case |
2 participants
PR checklist:
proton: starts/endsfor new code in existing community code base ?Please write user-readable short description of the changes: