ZTE-EBASE opened a new pull request, #1931: URL: https://github.com/apache/cloudberry/pull/1931
<!-- Thank you for your contribution to Apache Cloudberry (Incubating)! --> Fixes #ISSUE_Number ## What does this PR do? Fix putIntoUnackQueueRing function logic for Intercontect interfaces ### what There are several logic issues in the putIntoUnackQueueRing function. Defect 1: Redundant idx calculation (Severity: Low-Medium) Defect 2: Misuse of TIMER_SPAN_LOSS (Severity: Medium) Defect 3: Redundant conditional judgment (Severity: Very Low) ### why For Defect 1 (Redundant idx calculation): Performance waste: Each call executes one extra integer division and modulo operation (~5-10ns per call) Poor maintainability: If someone modifies one formula but forgets the other, it introduces hard-to-debug bugs Code confusion: Readers wonder "why calculate twice?" Misleading logs: Line 7054's log shows the first calculation, but line 7058's result is actually used For Defect 2 (TIMER_SPAN_LOSS misuse): Time alignment bias: Expected 5ms boundary alignment, actual 2.5ms alignment Imprecise retransmission timing: May cause packets to trigger retransmission too early or too late Degraded flow control performance: Affects accuracy of timeout-based mechanisms Subtle bug: Not immediately obvious during testing but impacts long-term stability For Defect 3 (Redundant conditional): Reduced readability: Unnecessary complexity makes code harder to understand Minor performance overhead: Extra conditional check serves no purpose Violates "don't repeat yourself" principle: The condition is checked twice unnecessarily Overall justification: These are not just cosmetic issues—they affect correctness (Defect 2) and code quality (all three) Low risk fix with high confidence: changes are localized and don't alter external behavior Follows best practices: clean code, eliminate redundancy, use correct constants ### How Perform logical fixes on the premise that existing code functionality remains intact. ### Type of Change - [ ] Bug fix (non-breaking change) - [ ] New feature (non-breaking change) - [ ] Breaking change (fix or feature with breaking changes) - [ ] Documentation update ### Breaking Changes <!-- Remove if not applicable. If yes, explain impact and migration path --> ### Test Plan <!-- How did you test these changes? --> - [ ] Unit tests added/updated - [ ] Integration tests added/updated - [ ] Passed `make installcheck` - [ ] Passed `make -C src/test installcheck-cbdb-parallel` ### Impact <!-- Remove sections that don't apply --> **Performance:** <!-- Any performance implications? --> **User-facing changes:** <!-- Any changes visible to users? --> **Dependencies:** <!-- New dependencies or version changes? --> ### Checklist - [ ] Followed [contribution guide](https://cloudberry.apache.org/contribute/code) - [ ] Added/updated documentation - [ ] Reviewed code for security implications - [ ] This PR contains AI-assisted code generation - [ ] Requested review from [cloudberry committers](https://github.com/orgs/apache/teams/cloudberry-committers) ### Additional Context <!-- Any other information that would help reviewers? Remove if none --> ### CI Skip Instructions <!-- To skip CI builds, add the appropriate CI skip identifier to your PR title. The identifier must: - Be in square brackets [] - Include the word "ci" and either "skip" or "no" - Only use for documentation-only changes or when absolutely necessary --> --- <!-- Join our community: - Mailing list: [[email protected]](https://lists.apache.org/[email protected]) (subscribe: [email protected]) - Discussions: https://github.com/apache/cloudberry/discussions --> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
