Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#1608][part-6][FOLLOWUP] improvement(client): Check blockId num after blocks all sent #1761

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

zuston
Copy link
Member

@zuston zuston commented Jun 3, 2024

What changes were proposed in this pull request?

  1. Rework the blockId number check in client side after all blocks are sent

Why are the changes needed?

The subtask of #1608

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

@@ -270,8 +270,8 @@ private void writeImpl(Iterator<Product2<K, V>> records) {
long s = System.currentTimeMillis();
checkAllBufferSpilled();
checkSentRecordCount(recordCount);
checkBlockSendResult(new HashSet<>(blockIds));
Copy link
Member Author

Choose a reason for hiding this comment

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

The variant of blockIds will be used for the checkSentBlockIds , so it's better to avoid modification in checkBlockSendResult

Copy link

github-actions bot commented Jun 3, 2024

Test Results

 2 433 files  ±0   2 433 suites  ±0   5h 0m 27s ⏱️ - 1m 14s
   934 tests ±0     933 ✅ ±0   1 💤 ±0  0 ❌ ±0 
10 828 runs  ±0  10 814 ✅ ±0  14 💤 ±0  0 ❌ ±0 

Results for commit 1e5321b. ± Comparison against base commit eb164a8.

@zuston zuston requested a review from jerqi June 4, 2024 02:17
@zuston
Copy link
Member Author

zuston commented Jun 4, 2024

PTAL @jerqi

@zuston
Copy link
Member Author

zuston commented Jun 11, 2024

ping @jerqi

Copy link
Contributor

@jerqi jerqi left a comment

Choose a reason for hiding this comment

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

LGTM.

@zuston zuston merged commit 6928080 into apache:master Jun 11, 2024
41 checks passed
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.

None yet

2 participants