Skip to content

Pipe: Fix stuck caused by async connector client not returned after transferring tsfiles & Fix validateTsFile and shouldMarkAsPipeRequest may not be effective (#15245)#15249

Merged
SteveYurongSu merged 1 commit intomasterfrom
fix-pipe-client-not-return-master
Apr 1, 2025

Conversation

@SteveYurongSu
Copy link
Member

No description provided.

…ransferring tsfiles & Fix validateTsFile and shouldMarkAsPipeRequest may not be effective (#15245)
@SteveYurongSu SteveYurongSu self-assigned this Apr 1, 2025
@SteveYurongSu SteveYurongSu requested a review from Copilot April 1, 2025 10:16
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request fixes an issue where the async connector client was not correctly returned to the pool after transferring tsfiles and addresses the ineffectiveness of validateTsFile and shouldMarkAsPipeRequest.

  • Revised the client handling logic to ensure its return using a dedicated method.
  • Enhanced error logging with detailed event counts in the retry exception.
  • Updated the receiverAttributes format in the client manager to include additional attributes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
PipeTransferTsFileHandler.java Consolidated client return logic via returnClientIfNecessary and added null checks.
IoTDBDataRegionAsyncConnector.java Updated exception message with additional details on failed retry events.
IoTDBDataNodeAsyncClientManager.java Extended receiverAttributes format to include validateTsFile and shouldMarkAsPipeRequest.
Comments suppressed due to low confidence (2)

iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/connector/protocol/thrift/async/handler/PipeTransferTsFileHandler.java:416

  • The returnClientIfNecessary method sets the client to null after returning it, which could lead to race conditions if accessed concurrently. Consider synchronizing this method to ensure thread-safety when modifying the client field.
private void returnClientIfNecessary() {

iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/connector/protocol/thrift/async/IoTDBDataRegionAsyncConnector.java:528

  • [nitpick] The enriched exception message now includes specific details about the remaining events. Verify that this detailed message does not inadvertently expose sensitive information and remains clear for end users.
throw new PipeException(

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 1, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@codecov
Copy link

codecov bot commented Apr 1, 2025

Codecov Report

Attention: Patch coverage is 0% with 28 lines in your changes missing coverage. Please review.

Project coverage is 39.40%. Comparing base (304fbab) to head (39854d7).
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
...hrift/async/handler/PipeTransferTsFileHandler.java 0.00% 22 Missing ⚠️
...ol/thrift/async/IoTDBDataRegionAsyncConnector.java 0.00% 4 Missing ⚠️
...nector/client/IoTDBDataNodeAsyncClientManager.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #15249      +/-   ##
============================================
- Coverage     39.40%   39.40%   -0.01%     
  Complexity      193      193              
============================================
  Files          4624     4624              
  Lines        297787   297808      +21     
  Branches      37174    37176       +2     
============================================
- Hits         117347   117344       -3     
- Misses       180440   180464      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@SteveYurongSu SteveYurongSu merged commit 91d067b into master Apr 1, 2025
52 of 53 checks passed
@SteveYurongSu SteveYurongSu deleted the fix-pipe-client-not-return-master branch April 1, 2025 11:47
JackieTien97 pushed a commit that referenced this pull request Apr 14, 2025
…ransferring tsfiles & Fix validateTsFile and shouldMarkAsPipeRequest may not be effective (#15245) (#15249)

(cherry picked from commit 91d067b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants