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/06/01 16:48:12 UTC
[GitHub] [accumulo] jmark99 opened a new pull request #2126: Unit test converting client props to BatchWriterConfig
jmark99 opened a new pull request #2126:
URL: https://github.com/apache/accumulo/pull/2126
Created two unit tests for the conversion of client properties to BatchWriterConfig. One using default values and another using modified values.
Refactored getBatchWriterConfig as suggested in initial ticket.
Added code in getBatchWriterConfig to allow for updating BATCH_WRTIER_THREADS_MAX as it was missing in the method as it existed.
--
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 #2126: Unit test converting client props to BatchWriterConfig
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2126:
URL: https://github.com/apache/accumulo/pull/2126#issuecomment-852299745
This may be better handled in a separate PR but, it looks like we could add very similar tests for ConditionalWriterConfig.
--
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 #2126: Unit test converting client props to BatchWriterConfig
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2126:
URL: https://github.com/apache/accumulo/pull/2126#discussion_r643343860
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java
##########
@@ -284,31 +284,36 @@ public SaslConnectionParams getSaslParams() {
return saslSupplier.get();
}
- public synchronized BatchWriterConfig getBatchWriterConfig() {
+ public synchronized static BatchWriterConfig getBatchWriterConfig(Properties props) {
Review comment:
Synchronized should go with the instance method and not the static 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a change in pull request #2126: Unit test converting client props to BatchWriterConfig
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #2126:
URL: https://github.com/apache/accumulo/pull/2126#discussion_r643514748
##########
File path: core/src/test/java/org/apache/accumulo/core/clientImpl/ClientContextTest.java
##########
@@ -90,4 +95,96 @@ public void sensitivePropertiesIncludedInProperties() {
assertFalse(props.containsKey("ignored.property"));
assertEquals("mysecret", props.get(Property.INSTANCE_SECRET.getKey()));
}
+
+ @Test
+ public void testGetBatchWriterConfigUsingDefaults() {
+ Properties props = new Properties();
+ BatchWriterConfig batchWriterConfig = ClientContext.getBatchWriterConfig(props);
+ assertNotNull(batchWriterConfig);
+
+ long expectedMemory = ConfigurationTypeHelper
+ .getMemoryAsBytes(ClientProperty.BATCH_WRITER_MEMORY_MAX.getDefaultValue());
+ assertEquals(expectedMemory, batchWriterConfig.getMaxMemory());
+
+ // If the value of BATCH_WRITE_LATENCY_MAX or BATCH_WRITER_TIMEOUT_MAX, is set to zero,
+ // Long.MAX_VALUE is returned. Effectively, this will cause data to be held in memory
+ // indefinitely for BATCH_WRITE_LATENCY_MAX and for no timeout, for BATCH_WRITER_TIMEOUT_MAX.
+ // Due to this behavior, the test compares the return values differently. If a value of
+ // 0 is used, compare the return value using TimeUnit.MILLISECONDS, otherwise the value
+ // should be converted to seconds in order to match the value set in ClientProperty.
+ long expectedLatency = ConfigurationTypeHelper
+ .getTimeInMillis(ClientProperty.BATCH_WRITER_LATENCY_MAX.getDefaultValue());
+ if (expectedLatency == 0) {
+ expectedLatency = Long.MAX_VALUE;
+ assertEquals(expectedLatency, batchWriterConfig.getMaxLatency(TimeUnit.MILLISECONDS));
+ } else {
+ assertEquals(expectedLatency, batchWriterConfig.getMaxLatency(TimeUnit.SECONDS));
+ }
+
+ long expectedTimeout = ConfigurationTypeHelper
+ .getTimeInMillis(ClientProperty.BATCH_WRITER_TIMEOUT_MAX.getDefaultValue());
+ if (expectedTimeout == 0) {
+ expectedTimeout = Long.MAX_VALUE;
+ assertEquals(expectedTimeout, batchWriterConfig.getTimeout(TimeUnit.MILLISECONDS));
+ } else {
+ assertEquals(expectedTimeout, batchWriterConfig.getTimeout(TimeUnit.SECONDS));
+ }
+
+ int expectedThreads =
+ Integer.parseInt(ClientProperty.BATCH_WRITER_THREADS_MAX.getDefaultValue());
+ assertEquals(expectedThreads, batchWriterConfig.getMaxWriteThreads());
+
+ Durability expectedDurability =
+ Durability.valueOf(ClientProperty.BATCH_WRITER_DURABILITY.getDefaultValue().toUpperCase());
+ assertEquals(expectedDurability, batchWriterConfig.getDurability());
+ }
+
+ @Test
+ public void testGetBatchWriterConfigNotUsingDefaults() {
+ Properties props = new Properties();
+
+ // set properties to non-default values
+ props.setProperty(ClientProperty.BATCH_WRITER_MEMORY_MAX.getKey(), "10M");
+ props.setProperty(ClientProperty.BATCH_WRITER_LATENCY_MAX.getKey(), "0");
+ props.setProperty(ClientProperty.BATCH_WRITER_TIMEOUT_MAX.getKey(), "15");
+ props.setProperty(ClientProperty.BATCH_WRITER_THREADS_MAX.getKey(), "12");
+ props.setProperty(ClientProperty.BATCH_WRITER_DURABILITY.getKey(), Durability.FLUSH.name());
+
+ BatchWriterConfig batchWriterConfig = ClientContext.getBatchWriterConfig(props);
+ assertNotNull(batchWriterConfig);
+
+ long expectedMemory = ConfigurationTypeHelper
+ .getMemoryAsBytes(ClientProperty.BATCH_WRITER_MEMORY_MAX.getValue(props));
+ assertEquals(expectedMemory, batchWriterConfig.getMaxMemory());
+
+ // If the value of BATCH_WRITE_LATENCY_MAX or BATCH_WRITER_TIMEOUT_MAX, is set to zero,
+ // Long.MAX_VALUE is returned. Effectively, this will cause data to be held in memory
+ // indefinitely for BATCH_WRITE_LATENCY_MAX and for no timeout, for BATCH_WRITER_TIMEOUT_MAX.
+ // Due to this behavior, the test compares the return values differently. If a value of
+ // 0 is used, compare the return value using TimeUnit.MILLISECONDS, otherwise the value
+ // should be converted to seconds in order to match the value set in ClientProperty.
+ long expectedLatency = ClientProperty.BATCH_WRITER_LATENCY_MAX.getTimeInMillis(props);
+ if (expectedLatency == 0) {
+ expectedLatency = Long.MAX_VALUE;
+ assertEquals(expectedLatency, batchWriterConfig.getMaxLatency(TimeUnit.MILLISECONDS));
+ } else {
+ assertEquals(expectedLatency, batchWriterConfig.getMaxLatency(TimeUnit.SECONDS));
+ }
Review comment:
Seems like since we know what the prop was set to the we can just check for that.
```suggestion
assertEquals(Long.MAX_VALUE, batchWriterConfig.getMaxLatency(TimeUnit.MILLISECONDS));
```
##########
File path: core/src/test/java/org/apache/accumulo/core/clientImpl/ClientContextTest.java
##########
@@ -90,4 +95,96 @@ public void sensitivePropertiesIncludedInProperties() {
assertFalse(props.containsKey("ignored.property"));
assertEquals("mysecret", props.get(Property.INSTANCE_SECRET.getKey()));
}
+
+ @Test
+ public void testGetBatchWriterConfigUsingDefaults() {
+ Properties props = new Properties();
+ BatchWriterConfig batchWriterConfig = ClientContext.getBatchWriterConfig(props);
+ assertNotNull(batchWriterConfig);
+
+ long expectedMemory = ConfigurationTypeHelper
+ .getMemoryAsBytes(ClientProperty.BATCH_WRITER_MEMORY_MAX.getDefaultValue());
+ assertEquals(expectedMemory, batchWriterConfig.getMaxMemory());
+
+ // If the value of BATCH_WRITE_LATENCY_MAX or BATCH_WRITER_TIMEOUT_MAX, is set to zero,
+ // Long.MAX_VALUE is returned. Effectively, this will cause data to be held in memory
+ // indefinitely for BATCH_WRITE_LATENCY_MAX and for no timeout, for BATCH_WRITER_TIMEOUT_MAX.
+ // Due to this behavior, the test compares the return values differently. If a value of
+ // 0 is used, compare the return value using TimeUnit.MILLISECONDS, otherwise the value
+ // should be converted to seconds in order to match the value set in ClientProperty.
+ long expectedLatency = ConfigurationTypeHelper
+ .getTimeInMillis(ClientProperty.BATCH_WRITER_LATENCY_MAX.getDefaultValue());
+ if (expectedLatency == 0) {
+ expectedLatency = Long.MAX_VALUE;
+ assertEquals(expectedLatency, batchWriterConfig.getMaxLatency(TimeUnit.MILLISECONDS));
+ } else {
+ assertEquals(expectedLatency, batchWriterConfig.getMaxLatency(TimeUnit.SECONDS));
+ }
+
+ long expectedTimeout = ConfigurationTypeHelper
+ .getTimeInMillis(ClientProperty.BATCH_WRITER_TIMEOUT_MAX.getDefaultValue());
+ if (expectedTimeout == 0) {
+ expectedTimeout = Long.MAX_VALUE;
+ assertEquals(expectedTimeout, batchWriterConfig.getTimeout(TimeUnit.MILLISECONDS));
+ } else {
+ assertEquals(expectedTimeout, batchWriterConfig.getTimeout(TimeUnit.SECONDS));
+ }
+
+ int expectedThreads =
+ Integer.parseInt(ClientProperty.BATCH_WRITER_THREADS_MAX.getDefaultValue());
+ assertEquals(expectedThreads, batchWriterConfig.getMaxWriteThreads());
+
+ Durability expectedDurability =
+ Durability.valueOf(ClientProperty.BATCH_WRITER_DURABILITY.getDefaultValue().toUpperCase());
+ assertEquals(expectedDurability, batchWriterConfig.getDurability());
+ }
+
+ @Test
+ public void testGetBatchWriterConfigNotUsingDefaults() {
+ Properties props = new Properties();
+
+ // set properties to non-default values
+ props.setProperty(ClientProperty.BATCH_WRITER_MEMORY_MAX.getKey(), "10M");
+ props.setProperty(ClientProperty.BATCH_WRITER_LATENCY_MAX.getKey(), "0");
+ props.setProperty(ClientProperty.BATCH_WRITER_TIMEOUT_MAX.getKey(), "15");
+ props.setProperty(ClientProperty.BATCH_WRITER_THREADS_MAX.getKey(), "12");
+ props.setProperty(ClientProperty.BATCH_WRITER_DURABILITY.getKey(), Durability.FLUSH.name());
+
+ BatchWriterConfig batchWriterConfig = ClientContext.getBatchWriterConfig(props);
+ assertNotNull(batchWriterConfig);
+
+ long expectedMemory = ConfigurationTypeHelper
+ .getMemoryAsBytes(ClientProperty.BATCH_WRITER_MEMORY_MAX.getValue(props));
+ assertEquals(expectedMemory, batchWriterConfig.getMaxMemory());
+
+ // If the value of BATCH_WRITE_LATENCY_MAX or BATCH_WRITER_TIMEOUT_MAX, is set to zero,
+ // Long.MAX_VALUE is returned. Effectively, this will cause data to be held in memory
+ // indefinitely for BATCH_WRITE_LATENCY_MAX and for no timeout, for BATCH_WRITER_TIMEOUT_MAX.
+ // Due to this behavior, the test compares the return values differently. If a value of
+ // 0 is used, compare the return value using TimeUnit.MILLISECONDS, otherwise the value
+ // should be converted to seconds in order to match the value set in ClientProperty.
+ long expectedLatency = ClientProperty.BATCH_WRITER_LATENCY_MAX.getTimeInMillis(props);
+ if (expectedLatency == 0) {
+ expectedLatency = Long.MAX_VALUE;
+ assertEquals(expectedLatency, batchWriterConfig.getMaxLatency(TimeUnit.MILLISECONDS));
+ } else {
+ assertEquals(expectedLatency, batchWriterConfig.getMaxLatency(TimeUnit.SECONDS));
+ }
+
+ long expectedTimeout = ClientProperty.BATCH_WRITER_TIMEOUT_MAX.getTimeInMillis(props);
+ if (expectedTimeout == 0) {
+ expectedTimeout = Long.MAX_VALUE;
+ assertEquals(expectedTimeout, batchWriterConfig.getTimeout(TimeUnit.MILLISECONDS));
+ } else {
+ assertEquals(expectedTimeout, batchWriterConfig.getTimeout(TimeUnit.SECONDS));
+ }
Review comment:
```suggestion
assertEquals(15, batchWriterConfig.getTimeout(TimeUnit.SECONDS));
```
--
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] jmark99 commented on pull request #2126: Unit test converting client props to BatchWriterConfig
Posted by GitBox <gi...@apache.org>.
jmark99 commented on pull request #2126:
URL: https://github.com/apache/accumulo/pull/2126#issuecomment-853025151
Thanks @DomGarguilo
--
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 #2126: Unit test converting client props to BatchWriterConfig
Posted by GitBox <gi...@apache.org>.
DomGarguilo commented on pull request #2126:
URL: https://github.com/apache/accumulo/pull/2126#issuecomment-853021301
I created #2131 after looking into this.
--
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] ctubbsii commented on a change in pull request #2126: Unit test converting client props to BatchWriterConfig
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2126:
URL: https://github.com/apache/accumulo/pull/2126#discussion_r643476077
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java
##########
@@ -284,31 +284,36 @@ public SaslConnectionParams getSaslParams() {
return saslSupplier.get();
}
+ public static BatchWriterConfig getBatchWriterConfig(Properties props) {
Review comment:
This shouldn't need to be public in order to be unit tested.
```suggestion
static BatchWriterConfig getBatchWriterConfig(Properties props) {
```
--
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] jmark99 commented on a change in pull request #2126: Unit test converting client props to BatchWriterConfig
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2126:
URL: https://github.com/apache/accumulo/pull/2126#discussion_r643347062
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java
##########
@@ -284,31 +284,36 @@ public SaslConnectionParams getSaslParams() {
return saslSupplier.get();
}
- public synchronized BatchWriterConfig getBatchWriterConfig() {
+ public synchronized static BatchWriterConfig getBatchWriterConfig(Properties props) {
Review comment:
Updated
--
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] jmark99 merged pull request #2126: Unit test converting client props to BatchWriterConfig
Posted by GitBox <gi...@apache.org>.
jmark99 merged pull request #2126:
URL: https://github.com/apache/accumulo/pull/2126
--
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