You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/08/11 17:37:28 UTC
[GitHub] [accumulo] milleruntime opened a new pull request, #2874: Add empty method to CloneConfiguration API
milleruntime opened a new pull request, #2874:
URL: https://github.com/apache/accumulo/pull/2874
* Add new convenience method for empty object and use in CloneTestIT
* Create private methods for sharing in TableOperationsImpl
* Clean up on the CloneConfiguration interface
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2874: Add empty method to CloneConfiguration API
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2874:
URL: https://github.com/apache/accumulo/pull/2874#discussion_r943757785
##########
core/src/main/java/org/apache/accumulo/core/client/admin/CloneConfiguration.java:
##########
@@ -98,21 +98,29 @@ public static interface Builder {
* @param keepOffline
* true if the new table is to be kept offline after cloning.
*/
- public Builder setKeepOffline(boolean keepOffline);
+ Builder setKeepOffline(boolean keepOffline);
/**
* Build the clone configuration
*
* @return the built immutable clone configuration
*/
- public CloneConfiguration build();
+ CloneConfiguration build();
}
/**
* @return a {@link CloneConfiguration} builder
*/
- public static CloneConfiguration.Builder builder() {
+ static CloneConfiguration.Builder builder() {
return new CloneConfigurationImpl();
}
+ /**
+ * @return an empty configuration object with the default settings.
+ * @since 2.1.0
+ */
+ static CloneConfiguration empty() {
Review Comment:
Could a more descriptive method name be used here? Maybe something along the lines of `emptyConfig` or `getEmptyCloneConfig`? Regardless, these changes LGTM
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime merged pull request #2874: Add empty method to CloneConfiguration API
Posted by GitBox <gi...@apache.org>.
milleruntime merged PR #2874:
URL: https://github.com/apache/accumulo/pull/2874
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a diff in pull request #2874: Add empty method to CloneConfiguration API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on code in PR #2874:
URL: https://github.com/apache/accumulo/pull/2874#discussion_r943774955
##########
core/src/main/java/org/apache/accumulo/core/client/admin/CloneConfiguration.java:
##########
@@ -98,21 +98,29 @@ public static interface Builder {
* @param keepOffline
* true if the new table is to be kept offline after cloning.
*/
- public Builder setKeepOffline(boolean keepOffline);
+ Builder setKeepOffline(boolean keepOffline);
/**
* Build the clone configuration
*
* @return the built immutable clone configuration
*/
- public CloneConfiguration build();
+ CloneConfiguration build();
}
/**
* @return a {@link CloneConfiguration} builder
*/
- public static CloneConfiguration.Builder builder() {
+ static CloneConfiguration.Builder builder() {
return new CloneConfigurationImpl();
}
+ /**
+ * @return an empty configuration object with the default settings.
+ * @since 2.1.0
+ */
+ static CloneConfiguration empty() {
Review Comment:
I wanted to keep it as short as possible since it's likely to be used as a param to the clone method.
<pre>
client.tableOperations().clone(table1, table2, CloneConfiguration.empty()));
</pre>
I also thought `none()` or `default()` might work. Or we could even just have a static `build()` but I thought that might be confusing with the `builder()` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a diff in pull request #2874: Add empty method to CloneConfiguration API
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on code in PR #2874:
URL: https://github.com/apache/accumulo/pull/2874#discussion_r943783524
##########
core/src/main/java/org/apache/accumulo/core/client/admin/CloneConfiguration.java:
##########
@@ -98,21 +98,29 @@ public static interface Builder {
* @param keepOffline
* true if the new table is to be kept offline after cloning.
*/
- public Builder setKeepOffline(boolean keepOffline);
+ Builder setKeepOffline(boolean keepOffline);
/**
* Build the clone configuration
*
* @return the built immutable clone configuration
*/
- public CloneConfiguration build();
+ CloneConfiguration build();
}
/**
* @return a {@link CloneConfiguration} builder
*/
- public static CloneConfiguration.Builder builder() {
+ static CloneConfiguration.Builder builder() {
return new CloneConfigurationImpl();
}
+ /**
+ * @return an empty configuration object with the default settings.
+ * @since 2.1.0
+ */
+ static CloneConfiguration empty() {
Review Comment:
Understood. Makes sense to me :+1:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2874: Add empty method to CloneConfiguration API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2874:
URL: https://github.com/apache/accumulo/pull/2874#issuecomment-1212370982
These changes were inspired by working on https://github.com/apache/accumulo/issues/2849
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on pull request #2874: Add empty method to CloneConfiguration API
Posted by GitBox <gi...@apache.org>.
milleruntime commented on PR #2874:
URL: https://github.com/apache/accumulo/pull/2874#issuecomment-1213071945
Merging this so I can continue working on #2849. If there is anymore feedback, I can do it as a follow on.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org