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

Core: Add check-status config to TableProperties. #2596

Merged
merged 3 commits into from
May 21, 2021

Conversation

coolderli
Copy link
Contributor

Currently, the minWaitMs and maxWaitMs are 1000 by default, we should add the configs of check-status to TableProperties.
I don't know if the default values I configured are appropriate.

@rdblue
Copy link
Contributor

rdblue commented May 19, 2021

This looks good to me, but we should clean up the property name if it hasn't been in a release yet.

@coolderli
Copy link
Contributor Author

@rdblue @aokolnychyi I have checked the latest tag 0.11.1, the commit.num-status-checks has not been released yet. I have renamed it to commit.status-checks.num-retries.

@@ -36,9 +36,18 @@ private TableProperties() {
public static final String COMMIT_TOTAL_RETRY_TIME_MS = "commit.retry.total-timeout-ms";
public static final int COMMIT_TOTAL_RETRY_TIME_MS_DEFAULT = 1800000; // 30 minutes

public static final String COMMIT_NUM_STATUS_CHECKS = "commit.num-status-checks";
public static final String COMMIT_NUM_STATUS_CHECKS = "commit.status-checks.num-retries";
Copy link
Contributor

Choose a reason for hiding this comment

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

The other properties are in commit.status-check, not commit.status-checks. Could you remove the extra s?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I could do it to the PR through github. I'll merge when tests are passing.

@rdblue rdblue merged commit 408a12a into apache:master May 21, 2021
@rdblue
Copy link
Contributor

rdblue commented May 21, 2021

Merged! Can you also update the table properties docs, @coolderli?

@coolderli
Copy link
Contributor Author

@rdblue OK, I will add the document as soon as possible

@coolderli
Copy link
Contributor Author

@rdblue #2661
I have updated the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants