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

Refactor XXXAdmin #317

Merged
merged 2 commits into from
Sep 29, 2021
Merged

Refactor XXXAdmin #317

merged 2 commits into from
Sep 29, 2021

Conversation

brfrn169
Copy link
Collaborator

What I did in this PR is:

  • Unify XXXAdmin and XXXTableMetadataManager because the difference of the two roles are getting ambiguous
  • Make the role of TableMetadataManager only for caching table metadata
  • Remove ConnectionException and StorageRuntimeException

Please take a look!

@brfrn169 brfrn169 self-assigned this Sep 28, 2021
feeblefakie
feeblefakie previously approved these changes Sep 29, 2021
Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you! It looks better 👍
I left one minor comment but it doesn't block it from being merged.

@@ -173,7 +173,9 @@ public void mutate(List<? extends Mutation> mutations) throws ExecutionException

Utility.setTargetToIfNot(mutations, namespacePrefix, namespace, tableName);
operationChecker.check(mutations);
mutations.forEach(operationChecker::check);
for (Mutation mutation : mutations) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason for not using forEach?

Copy link
Collaborator Author

@brfrn169 brfrn169 Sep 29, 2021

Choose a reason for hiding this comment

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

Thank you for taking at this!

After this change, operationChecker::check throws ExecutionException. forEach doesn't handle Exception well, so I changed this way.

@feeblefakie feeblefakie merged commit f683d7a into master Sep 29, 2021
@feeblefakie feeblefakie deleted the refactor-XXXAdmin branch September 29, 2021 06:26
brfrn169 added a commit that referenced this pull request Oct 31, 2021
brfrn169 added a commit that referenced this pull request Nov 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants