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 2021/05/24 14:46:45 UTC
[GitHub] [accumulo] DomGarguilo opened a new pull request #2116: ConditionalWriterConfig - update create method and props gathering
DomGarguilo opened a new pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116
Closes #680
Two main changes:
- Create an overloaded createConditionalMutation(String tableName) method which creates a new ConditionalWriterConfig() so the config is optional to pass
- Allow client properties to be set in the client properties file
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2116: ConditionalWriterConfig - update create method and props gathering
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116#discussion_r638040748
##########
File path: core/src/main/java/org/apache/accumulo/core/client/ConditionalWriterConfig.java
##########
@@ -175,4 +190,32 @@ public String getClassLoaderContext() {
return this.classLoaderContext;
}
+ private static <T> T merge(T o1, T o2) {
+ if (o1 != null)
+ return o1;
+ return o2;
+ }
+
+ /**
+ * Merge this ConditionalWriterConfig with another. If config is set in both, preference will be
+ * given to this config.
+ *
+ * @param other
+ * Another ConditionalWriterConfig
+ * @return Merged ConditionalWriterConfig
+ * @since 2.0.0
+ */
+ public ConditionalWriterConfig merge(ConditionalWriterConfig other) {
+ ConditionalWriterConfig result = new ConditionalWriterConfig();
+ result.timeout = merge(this.timeout, other.timeout);
+ result.maxWriteThreads = merge(this.maxWriteThreads, other.maxWriteThreads);
+ result.auths = merge(this.auths, other.auths);
Review comment:
Auths is initialized to a non-null value, so not sure merge will do anything.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2116: ConditionalWriterConfig - update create method and props gathering
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116#discussion_r638129053
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/ClientProperty.java
##########
@@ -74,6 +74,17 @@
+ " use the table's durability setting. ",
"2.0.0", false),
+ // ConditionalWriter
+ CONDITIONAL_WRITER_TIMEOUT_MAX("conditional.writer.timeout.max", "0", PropertyType.TIMEDURATION,
+ "Maximum amount of time an unresponsive server will be re-tried.", "2.0.0", false),
Review comment:
I was thinking of adding a sentence to `"Maximum amount of time an unresponsive server will be re-tried."`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner merged pull request #2116: ConditionalWriterConfig - update create method and props gathering
Posted by GitBox <gi...@apache.org>.
keith-turner merged pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2116: ConditionalWriterConfig - update create method and props gathering
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116#discussion_r638324679
##########
File path: core/src/main/java/org/apache/accumulo/core/client/ConditionalWriterConfig.java
##########
@@ -175,4 +190,32 @@ public String getClassLoaderContext() {
return this.classLoaderContext;
}
+ private static <T> T merge(T o1, T o2) {
+ if (o1 != null)
+ return o1;
+ return o2;
+ }
+
+ /**
+ * Merge this ConditionalWriterConfig with another. If config is set in both, preference will be
+ * given to this config.
+ *
+ * @param other
+ * Another ConditionalWriterConfig
+ * @return Merged ConditionalWriterConfig
+ * @since 2.0.0
+ */
+ public ConditionalWriterConfig merge(ConditionalWriterConfig other) {
+ ConditionalWriterConfig result = new ConditionalWriterConfig();
+ result.timeout = merge(this.timeout, other.timeout);
+ result.maxWriteThreads = merge(this.maxWriteThreads, other.maxWriteThreads);
+ result.auths = merge(this.auths, other.auths);
Review comment:
would it be possible to make the auths follow the pattern around the durability field?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2116: ConditionalWriterConfig - update create method and props gathering
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116#discussion_r638770606
##########
File path: core/src/main/java/org/apache/accumulo/core/client/ConditionalWriterConfig.java
##########
@@ -175,4 +190,32 @@ public String getClassLoaderContext() {
return this.classLoaderContext;
}
+ private static <T> T merge(T o1, T o2) {
+ if (o1 != null)
+ return o1;
+ return o2;
+ }
+
+ /**
+ * Merge this ConditionalWriterConfig with another. If config is set in both, preference will be
+ * given to this config.
+ *
+ * @param other
+ * Another ConditionalWriterConfig
+ * @return Merged ConditionalWriterConfig
+ * @since 2.0.0
+ */
+ public ConditionalWriterConfig merge(ConditionalWriterConfig other) {
+ ConditionalWriterConfig result = new ConditionalWriterConfig();
+ result.timeout = merge(this.timeout, other.timeout);
+ result.maxWriteThreads = merge(this.maxWriteThreads, other.maxWriteThreads);
+ result.auths = merge(this.auths, other.auths);
Review comment:
> would it be possible to make the auths follow the pattern around the durability field?
Do you mean initializing it as null similar to how we did with durability so it can be treated like the rest?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2116: ConditionalWriterConfig - update create method and props gathering
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116#discussion_r638839085
##########
File path: core/src/main/java/org/apache/accumulo/core/client/ConditionalWriterConfig.java
##########
@@ -175,4 +190,32 @@ public String getClassLoaderContext() {
return this.classLoaderContext;
}
+ private static <T> T merge(T o1, T o2) {
+ if (o1 != null)
+ return o1;
+ return o2;
+ }
+
+ /**
+ * Merge this ConditionalWriterConfig with another. If config is set in both, preference will be
+ * given to this config.
+ *
+ * @param other
+ * Another ConditionalWriterConfig
+ * @return Merged ConditionalWriterConfig
+ * @since 2.0.0
+ */
+ public ConditionalWriterConfig merge(ConditionalWriterConfig other) {
+ ConditionalWriterConfig result = new ConditionalWriterConfig();
+ result.timeout = merge(this.timeout, other.timeout);
+ result.maxWriteThreads = merge(this.maxWriteThreads, other.maxWriteThreads);
+ result.auths = merge(this.auths, other.auths);
Review comment:
> Do you mean initializing it as null similar to how we did with durability so it can be treated like the rest?
Yeah and in the getter return Auths.EMPTY when its null.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2116: ConditionalWriterConfig - update create method and props gathering
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116#discussion_r638025341
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java
##########
@@ -304,10 +305,35 @@ public BatchWriterConfig getBatchWriterConfig() {
if (!durability.isEmpty()) {
batchWriterConfig.setDurability(Durability.valueOf(durability.toUpperCase()));
}
+ Integer maxThreads = ClientProperty.BATCH_WRITER_THREADS_MAX.getInteger(props);
+ if (maxThreads != null) {
+ batchWriterConfig.setMaxWriteThreads(maxThreads);
+ }
}
return batchWriterConfig;
}
+ public ConditionalWriterConfig getConditionalWriterConfig() {
+ ensureOpen();
+ if (conditionalWriterConfig == null) {
+ Properties props = info.getProperties();
+ conditionalWriterConfig = new ConditionalWriterConfig();
Review comment:
Should probably make this method synchronized. Could have one thread getting this object while another is setting it up. May need to do the same for the batchwriter method.
##########
File path: core/src/main/java/org/apache/accumulo/core/client/AccumuloClient.java
##########
@@ -267,6 +267,18 @@ Scanner createScanner(String tableName)
ConditionalWriter createConditionalWriter(String tableName, ConditionalWriterConfig config)
throws TableNotFoundException;
+ /**
+ * Factory method to create a ConditionalWriter connected to Accumulo.
+ *
+ * @param tableName
+ * the name of the table to query data from
+ *
+ * @return ConditionalWriter object for writing ConditionalMutations
+ * @throws TableNotFoundException
+ * when the specified table doesn't exist
Review comment:
```suggestion
* when the specified table doesn't exist
*
* @since 2.1.0
```
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/ClientProperty.java
##########
@@ -74,6 +74,17 @@
+ " use the table's durability setting. ",
"2.0.0", false),
+ // ConditionalWriter
+ CONDITIONAL_WRITER_TIMEOUT_MAX("conditional.writer.timeout.max", "0", PropertyType.TIMEDURATION,
+ "Maximum amount of time an unresponsive server will be re-tried.", "2.0.0", false),
+ CONDITIONAL_WRITER_THREADS_MAX("conditional.writer.threads.max", "3", PropertyType.COUNT,
+ "Maximum number of threads to use for writing data to tablet servers.", "2.0.0", false),
+ CONDITIONAL_WRITER_DURABILITY("conditional.writer.durability", "default", PropertyType.DURABILITY,
+ Property.TABLE_DURABILITY.getDescription() + " Setting this property will "
+ + "change the durability for the ConditionalWriter session. A value of "
+ + "\"default\" will use the table's durability setting. ",
+ "2.0.0", false),
Review comment:
```suggestion
CONDITIONAL_WRITER_TIMEOUT_MAX("conditional.writer.timeout.max", "0", PropertyType.TIMEDURATION,
"Maximum amount of time an unresponsive server will be re-tried.", "2.1.0", false),
CONDITIONAL_WRITER_THREADS_MAX("conditional.writer.threads.max", "3", PropertyType.COUNT,
"Maximum number of threads to use for writing data to tablet servers.", "2.1.0", false),
CONDITIONAL_WRITER_DURABILITY("conditional.writer.durability", "default", PropertyType.DURABILITY,
Property.TABLE_DURABILITY.getDescription() + " Setting this property will "
+ "change the durability for the ConditionalWriter session. A value of "
+ "\"default\" will use the table's durability setting. ",
"2.1.0", false),
```
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/ClientProperty.java
##########
@@ -74,6 +74,17 @@
+ " use the table's durability setting. ",
"2.0.0", false),
+ // ConditionalWriter
+ CONDITIONAL_WRITER_TIMEOUT_MAX("conditional.writer.timeout.max", "0", PropertyType.TIMEDURATION,
+ "Maximum amount of time an unresponsive server will be re-tried.", "2.0.0", false),
Review comment:
Should document the behavior of zero, it seems to mean no timeout.
##########
File path: core/src/main/java/org/apache/accumulo/core/client/ConditionalWriterConfig.java
##########
@@ -175,4 +190,32 @@ public String getClassLoaderContext() {
return this.classLoaderContext;
}
+ private static <T> T merge(T o1, T o2) {
+ if (o1 != null)
+ return o1;
+ return o2;
+ }
+
+ /**
+ * Merge this ConditionalWriterConfig with another. If config is set in both, preference will be
+ * given to this config.
+ *
+ * @param other
+ * Another ConditionalWriterConfig
+ * @return Merged ConditionalWriterConfig
+ * @since 2.0.0
+ */
+ public ConditionalWriterConfig merge(ConditionalWriterConfig other) {
+ ConditionalWriterConfig result = new ConditionalWriterConfig();
+ result.timeout = merge(this.timeout, other.timeout);
+ result.maxWriteThreads = merge(this.maxWriteThreads, other.maxWriteThreads);
+ result.auths = merge(this.auths, other.auths);
+ if (this.isDurabilitySet) {
Review comment:
Could do the following so that `merge()` can also be used here.
* Start durability off as null.
* Make getDurability return DEFAULT when its null
* Drop isDurabilitySet
##########
File path: core/src/main/java/org/apache/accumulo/core/client/ConditionalWriterConfig.java
##########
@@ -175,4 +190,32 @@ public String getClassLoaderContext() {
return this.classLoaderContext;
}
+ private static <T> T merge(T o1, T o2) {
+ if (o1 != null)
+ return o1;
+ return o2;
+ }
+
+ /**
+ * Merge this ConditionalWriterConfig with another. If config is set in both, preference will be
+ * given to this config.
+ *
+ * @param other
+ * Another ConditionalWriterConfig
+ * @return Merged ConditionalWriterConfig
+ * @since 2.0.0
Review comment:
```suggestion
* @since 2.1.0
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2116: ConditionalWriterConfig - update create method and props gathering
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116#discussion_r638165618
##########
File path: core/src/main/java/org/apache/accumulo/core/client/ConditionalWriterConfig.java
##########
@@ -175,4 +190,32 @@ public String getClassLoaderContext() {
return this.classLoaderContext;
}
+ private static <T> T merge(T o1, T o2) {
+ if (o1 != null)
+ return o1;
+ return o2;
+ }
+
+ /**
+ * Merge this ConditionalWriterConfig with another. If config is set in both, preference will be
+ * given to this config.
+ *
+ * @param other
+ * Another ConditionalWriterConfig
+ * @return Merged ConditionalWriterConfig
+ * @since 2.0.0
+ */
+ public ConditionalWriterConfig merge(ConditionalWriterConfig other) {
+ ConditionalWriterConfig result = new ConditionalWriterConfig();
+ result.timeout = merge(this.timeout, other.timeout);
+ result.maxWriteThreads = merge(this.maxWriteThreads, other.maxWriteThreads);
+ result.auths = merge(this.auths, other.auths);
Review comment:
You're right. I can change it to `result.auths = this.auths;`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2116: ConditionalWriterConfig - update create method and props gathering
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116#discussion_r638111309
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/ClientProperty.java
##########
@@ -74,6 +74,17 @@
+ " use the table's durability setting. ",
"2.0.0", false),
+ // ConditionalWriter
+ CONDITIONAL_WRITER_TIMEOUT_MAX("conditional.writer.timeout.max", "0", PropertyType.TIMEDURATION,
+ "Maximum amount of time an unresponsive server will be re-tried.", "2.0.0", false),
Review comment:
Where do you think it should be documented? A comment?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on a change in pull request #2116: ConditionalWriterConfig - update create method and props gathering
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on a change in pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116#discussion_r638908355
##########
File path: core/src/main/java/org/apache/accumulo/core/client/ConditionalWriterConfig.java
##########
@@ -175,4 +190,32 @@ public String getClassLoaderContext() {
return this.classLoaderContext;
}
+ private static <T> T merge(T o1, T o2) {
+ if (o1 != null)
+ return o1;
+ return o2;
+ }
+
+ /**
+ * Merge this ConditionalWriterConfig with another. If config is set in both, preference will be
+ * given to this config.
+ *
+ * @param other
+ * Another ConditionalWriterConfig
+ * @return Merged ConditionalWriterConfig
+ * @since 2.0.0
+ */
+ public ConditionalWriterConfig merge(ConditionalWriterConfig other) {
+ ConditionalWriterConfig result = new ConditionalWriterConfig();
+ result.timeout = merge(this.timeout, other.timeout);
+ result.maxWriteThreads = merge(this.maxWriteThreads, other.maxWriteThreads);
+ result.auths = merge(this.auths, other.auths);
Review comment:
Included in the most recent commit
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] DomGarguilo commented on pull request #2116: ConditionalWriterConfig - update create method and props gathering
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2116:
URL: https://github.com/apache/accumulo/pull/2116#issuecomment-847277553
@keith-turner, I did my best to include your suggestions in the most recent commit, 9050c89
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org