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 15:09:15 UTC
[GitHub] [accumulo] keith-turner commented on a change in pull request #2116: ConditionalWriterConfig - update create method and props gathering
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