Skip to content

Conversation

@zuston
Copy link
Member

@zuston zuston commented Sep 14, 2022

What changes were proposed in this pull request?

Adjust the memory required times from 100 to 50 to match the grpc max deadline conf (60s).

Why are the changes needed?

If the required memory retry time > 60s, the client will cancel the request. That means the required memory exception wont throw to client side. And the server side wont show any related log.

Does this PR introduce any user-facing change?

No

How was this patch tested?

@codecov-commenter
Copy link

codecov-commenter commented Sep 14, 2022

Codecov Report

Merging #218 (ef2bfdc) into master (f6b07f8) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master     #218   +/-   ##
=========================================
  Coverage     59.13%   59.13%           
  Complexity     1327     1327           
=========================================
  Files           160      160           
  Lines          8727     8727           
  Branches        817      817           
=========================================
  Hits           5161     5161           
  Misses         3301     3301           
  Partials        265      265           
Impacted Files Coverage Δ
...a/org/apache/uniffle/server/ShuffleServerConf.java 99.18% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@roryqi
Copy link
Contributor

roryqi commented Sep 14, 2022

Like discussion at dev mail list, we will freeze the code and cut 0.6 version branch in September 15, we will not merge this pr before I cut 0.6 version branch, are you ok?

@zuston
Copy link
Member Author

zuston commented Sep 20, 2022

PTAL @jerqi

@roryqi
Copy link
Contributor

roryqi commented Sep 21, 2022

I can't get the point. Could you give me more explanation?

@zuston
Copy link
Member Author

zuston commented Sep 21, 2022

Now the require-memory retry threshold time > 60s in shuffle-server and the client grpc request max-time is 60s, it means that when the shuffle -shuffle memory is tight and the request time reached the 60s, the client side only will show the request timeout exception instead of requiring memory failed log.

@roryqi
Copy link
Contributor

roryqi commented Sep 21, 2022

Now the require-memory retry threshold time > 60s in shuffle-server and the client grpc request max-time is 60s, it means that when the shuffle -shuffle memory is tight and the request time reached the 60s, the client side only will show the request timeout exception instead of requiring memory failed log.

OK, make sense, Let's merge it, thanks @zuston

@roryqi roryqi merged commit f63cebb into apache:master Sep 21, 2022
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.

3 participants