Skip to content

Conversation

@lifeSo
Copy link
Collaborator

@lifeSo lifeSo commented Jan 7, 2024

What changes were proposed in this pull request?

Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION instead of NO_BUFFER

Why are the changes needed?

Fix: #1119

Does this PR introduce any user-facing change?

No.

How was this patch tested?

unit test

@lifeSo
Copy link
Collaborator Author

lifeSo commented Jan 7, 2024

The code is same as : #1393

This is clear(the 1393 is also ok), And I think I corrected all suggestion.

PLTA @zuston

@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2024

Codecov Report

Attention: 46 lines in your changes are missing coverage. Please review.

Comparison is base (15a399a) 53.43% compared to head (6833e35) 54.23%.

Files Patch % Lines
...pache/uniffle/server/ShuffleServerGrpcService.java 0.00% 17 Missing ⚠️
...ffle/client/impl/grpc/ShuffleServerGrpcClient.java 0.00% 9 Missing ⚠️
...he/uniffle/common/exception/NoBufferException.java 0.00% 6 Missing ⚠️
...n/exception/NoBufferForHugePartitionException.java 0.00% 6 Missing ⚠️
.../uniffle/common/exception/NoRegisterException.java 0.00% 6 Missing ⚠️
.../org/apache/uniffle/server/ShuffleTaskManager.java 81.81% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1425      +/-   ##
============================================
+ Coverage     53.43%   54.23%   +0.80%     
+ Complexity     2723     2720       -3     
============================================
  Files           419      402      -17     
  Lines         23992    21659    -2333     
  Branches       2046     2043       -3     
============================================
- Hits          12820    11747    -1073     
+ Misses        10378     9190    -1188     
+ Partials        794      722      -72     

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

Copy link
Member

@zuston zuston left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for your effort. @lifeSo


// Add NoBufferForHugePartitionException check
// and ShuffleServerMetrics.TOTAL_REQUIRE_BUFFER_FAILED_FOR_HUGE_PARTITION metric should be 1
String content = TestUtils.httpGet(SHUFFLE_SERVER_METRICS_URL);
Copy link
Member

Choose a reason for hiding this comment

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

This is really weird. Maybe we could optimize this in the another PR.

@zuston zuston changed the title [#1119] improvement: Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION instead of NO_BUFFER [#1119] improvement(client): Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION Jan 8, 2024
@zuston zuston merged commit 08e8b63 into apache:master Jan 8, 2024
@lifeSo lifeSo deleted the feature_1119_new_2024 branch January 8, 2024 06:54

gaugeIsHealthy = metricsManager.addLabeledGauge(IS_HEALTHY);
gaugeAllocatedBufferSize = metricsManager.addLabeledGauge(ALLOCATED_BUFFER_SIZE);
counterExpiredPreAllocatedBufferSizeTotal =
Copy link
Member

Choose a reason for hiding this comment

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

Why removing this?

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.

[Improvement] Explicitly throw BUFFER_LIMIT_OF_HUGE_PARTITION instead of NO_BUFFER

3 participants