Adjust the memory required times to match grpc max deadline conf by zuston · Pull Request #218 · apache/uniffle · GitHub
Skip to content

Adjust the memory required times to match grpc max deadline conf#218

Merged
roryqi merged 1 commit into
apache:masterfrom
zuston:retryTimes
Sep 21, 2022
Merged

Adjust the memory required times to match grpc max deadline conf#218
roryqi merged 1 commit into
apache:masterfrom
zuston:retryTimes

Conversation

@zuston

@zuston zuston commented Sep 14, 2022

Copy link
Copy Markdown
Member

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

codecov-commenter commented Sep 14, 2022

Copy link
Copy Markdown

@roryqi

roryqi commented Sep 14, 2022

Copy link
Copy Markdown
Contributor

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

zuston commented Sep 20, 2022

Copy link
Copy Markdown
Member Author

PTAL @jerqi

@roryqi

roryqi commented Sep 21, 2022

Copy link
Copy Markdown
Contributor

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

@zuston

zuston commented Sep 21, 2022

Copy link
Copy Markdown
Member Author

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

roryqi commented Sep 21, 2022

Copy link
Copy Markdown
Contributor

@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