Change kafka producer client config #2030
Conversation
WalkthroughThe recent changes enhance the configuration of the indexer and Kafka message sending functionality. Notably, the maximum retry limit for indexer operations has increased significantly, fostering improved resilience during failures. Additionally, new parameters for Kafka, including retry backoff timing and acknowledgment requirements, have been introduced, bolstering message delivery reliability and ensuring greater robustness in the overall system. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Indexer
participant Zookeeper
participant Kafka
Client->>Indexer: Send message
Indexer->>Zookeeper: Validate session
Zookeeper-->>Indexer: Session valid
Indexer->>Kafka: Send message with retries
Kafka->>Indexer: Acknowledgment of message
Indexer->>Client: Confirm receipt
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- protocol/indexer/flags.go (1 hunks)
- protocol/indexer/msgsender/msgsender_kafka.go (1 hunks)
Additional comments not posted (4)
protocol/indexer/flags.go (2)
25-25: LGTM!The added comment provides valuable context for the retry logic, linking it to the retry backoff strategy in relation to Zookeeper's session timeout.
19-19: LGTM! But verify the impact of increasing retries.The increase in
DefaultMaxRetriesfrom 3 to 20 can enhance robustness. However, ensure that this change does not lead to unintended side effects, such as prolonged retry times or resource exhaustion.Verification successful
LGTM! But verify the impact of increasing retries.
The increase in
DefaultMaxRetriesfrom 3 to 20 can enhance robustness. The existing tests inflags_test.go,msgsender_kafka_integration_test.go, andmsgsender_kafka_test.goshould cover scenarios involving the new retry limit. Ensure that these tests pass and that no issues related to prolonged retry times or resource exhaustion are observed.
protocol/indexer/flags_test.goprotocol/indexer/msgsender/msgsender_kafka_integration_test.goprotocol/indexer/msgsender/msgsender_kafka_test.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of increasing DefaultMaxRetries. # Test: Search for the usage of DefaultMaxRetries. Expect: Ensure that the increased retries do not cause issues. rg --type go -A 5 $'DefaultMaxRetries'Length of output: 3037
Script:
#!/bin/bash # Description: Verify if there are test cases that specifically check the behavior of the system with DefaultMaxRetries set to 20. # Test: Search for test cases that use DefaultMaxRetries. rg --type go -A 10 'DefaultMaxRetries' | rg -i 'test'Length of output: 4371
protocol/indexer/msgsender/msgsender_kafka.go (2)
49-49: LGTM!Setting the
RequiredAcksparameter tosarama.WaitForAllimproves the reliability of message delivery by ensuring that the producer waits for acknowledgment from all in-sync replicas before considering a message successfully sent.
46-47: LGTM! But verify the retry backoff strategy.Setting the
Retry.Backoffparameter to 1000 milliseconds ensures a delay between retry attempts, which can help in avoiding rapid retry failures. However, verify that this change aligns with the overall retry strategy and does not cause unintended delays.Verification successful
LGTM! But verify the retry backoff strategy.
Setting the
Retry.Backoffparameter to 1000 milliseconds ensures a delay between retry attempts, which can help in avoiding rapid retry failures. The overall retry strategy includes setting both the backoff duration and the maximum number of retries, which aligns with the intended design. Ensure that this change does not cause unintended delays.
protocol/indexer/msgsender/msgsender_kafka.go: ConfiguresRetry.BackoffandRetry.Max.protocol/indexer/flags.go: Defines the flag forMaxRetries.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the retry backoff strategy. # Test: Search for the usage of Retry.Backoff. Expect: Ensure that the backoff strategy aligns with the overall retry logic. rg --type go -A 5 $'Retry.Backoff'Length of output: 707
Script:
#!/bin/bash # Description: Verify the overall retry strategy by searching for other retry-related configurations. # Search for configurations related to retry attempts. rg --type go -A 5 'Retry' # Search for configurations related to the producer in the Kafka setup. rg --type go -A 5 'Producer' # Search for any documentation or comments that might provide context on the retry strategy. rg --type go -A 5 'retry'Length of output: 18676
| ) | ||
|
|
||
| // List of CLI flags | ||
| const ( | ||
| FlagKafkaConnStr = "indexer-kafka-conn-str" |
There was a problem hiding this comment.
Nit: I think this is a lint error?
There was a problem hiding this comment.
I think adding the comment underneath caused the linter to not add the spacing.
|
@Mergifyio backport release/protocol/v6.x |
✅ Backports have been created
|
(cherry picked from commit 44054d4)
|
@mergify backport release/protocol/v5.2.x |
✅ Backports have been created
|
(cherry picked from commit 44054d4)
Co-authored-by: jerryfan01234 <[email protected]>
Co-authored-by: jerryfan01234 <[email protected]>
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Retry.Backoffset to 1000 ms andRequiredAcksset tosarama.WaitForAll.Bug Fixes