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 2020/07/02 21:14:38 UTC
[GitHub] [accumulo] keith-turner opened a new pull request #1649: Addresses three compaction service follow on issues.
keith-turner opened a new pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649
While waiting for #1632 to be reviewed I completed multiple follow on issues for the new compaction service. I am submitting those as a single PR, but there is a commit for each. I can break them into multiple PRs if desired.
----------------------------------------------------------------
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 pull request #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#issuecomment-692755389
> This looks good, but why is default_any_running=1 but nothing in default large, medium or small? Is default_any a separate queue, or should it be the sum of all default_any_ ?
@EdColeman do you still have the source for the test that emitted these metrics? I would like to see the compaction config used in the test.
----------------------------------------------------------------
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] milleruntime commented on pull request #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#issuecomment-659375737
I would like @EdColeman to take a look at the metrics you added for compaction. I will look over the other commits
----------------------------------------------------------------
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 edited a comment on pull request #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner edited a comment on pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#issuecomment-720844642
Since this branch has been sitting for a while, locally I merged this branch to main and reran all ITs matching `*Compaction*IT*`. The test were all a success.
----------------------------------------------------------------
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 #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r456741131
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -415,6 +415,10 @@
TSERV_COMPACTION_SERVICE_ROOT_PLANNER("tserver.compaction.major.service.root.planner",
DefaultCompactionPlanner.class.getName(), PropertyType.CLASSNAME,
"Compaction planner for root tablet service"),
+ TSERV_COMPACTION_SERVICE_ROOT_THROUGHPUT("tserver.compaction.major.service.root.throughput", "0B",
+ PropertyType.BYTES,
+ "Maximum number of bytes to read or write per second over all major"
+ + " compactions in this compaction service, or 0B for unlimited."),
Review comment:
I like rate.limit better. Changed to rate.limit in abc961a
----------------------------------------------------------------
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 #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r464472045
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionExecutor.java
##########
@@ -94,8 +112,11 @@ public boolean cancel(Status expectedStatus) {
canceled = status.compareAndSet(expectedStatus, Status.CANCELED);
}
+ if (canceled)
+ queuedTask.remove(this);
+
if (canceled && cancelCount.incrementAndGet() % 1024 == 0) {
- // nMeed to occasionally clean the queue, it could have canceled task with low priority that
+ // need to occasionally clean the queue, it could have canceled task with low priority that
// hang around. Avoid cleaning it every time something is canceled as that could be
// expensive.
Review comment:
it refers to the queue.. I can clean this up
----------------------------------------------------------------
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 #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r464484830
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -415,6 +415,10 @@
TSERV_COMPACTION_SERVICE_ROOT_PLANNER("tserver.compaction.major.service.root.planner",
DefaultCompactionPlanner.class.getName(), PropertyType.CLASSNAME,
"Compaction planner for root tablet service"),
+ TSERV_COMPACTION_SERVICE_ROOT_RATE_LIMIT("tserver.compaction.major.service.root.rate.limit", "0B",
Review comment:
> > I worry that we have added far too many new configuration properties for compactions, and that this might create a terrible user experience around compactions.
>
> Could open a new issue and discuss a course of action.
For the larger issue, yes. But I'm struggling to keep up with the pace of all these changes, and am raising the point here, to step back and ask the more narrow question of whether these specific properties are needed. If they are, that's fine... we can move on. But, I don't know if I have the mental capacity to circle back and form a coherent discussion around the larger issue that a separate ticket would serve. The time to do that was probably several weeks ago before the wave of new properties got introduced, and that ship may have sailed.
>
> > Also, what does "root" refer to here for this property?
>
> This is a compaction service for the root table.
Okay, cool. Thanks. However, I suppose the point of my feedback wasn't to get a response here, but to get a response in the form of an improved description field in the code.
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r455788993
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/CompactCommand.java
##########
@@ -147,30 +170,28 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
compactionConfig.setIterators(new ArrayList<>(iterators));
}
- Map<String,String> selectorOpts = new HashMap<>();
- Map<String,String> configurerOpts = new HashMap<>();
- getConfigurableCompactionStrategyOpts(cl, selectorOpts, configurerOpts);
+ setupConfigurableCompaction(cl, compactionConfig);
if (cl.hasOption(strategyOpt.getOpt())) {
- if (!selectorOpts.isEmpty() || !configurerOpts.isEmpty())
+ if (cl.hasOption(selectorOpt.getLongOpt()) || cl.hasOption(configurerOpt.getLongOpt())) {
throw new IllegalArgumentException(
- "Can not specify compaction strategy with file selection and file output options.");
-
+ "Can not specify a strategy with a selector or configurer");
+ }
configureCompactionStrat(cl);
}
- if (!selectorOpts.isEmpty()) {
- PluginConfig selectorCfg = new PluginConfig(
- "org.apache.accumulo.tserver.compaction.strategies.ConfigurableCompactionStrategy",
- selectorOpts);
- compactionConfig.setSelector(selectorCfg);
+ if (cl.hasOption(selectorOpt.getLongOpt())) {
Review comment:
If the new options can't be set along with a strategy, shouldn't these two if statements be in an else?
----------------------------------------------------------------
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] EdColeman commented on pull request #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#issuecomment-692790665
Not source code as such - the configuration would have been the defaults.
What I do is hack one of the IT tests that use the slow compaction runner and then just insert a hard Thread.sleep(xxx) - I usually let the test fail because of timeout, so I have as much time as possible for things to report.
One candidate would be in org.apache.accumulo.test.functional.FateConcurrencyIT.multipleCompactions() at line 496 - right before the compactions are cancelled. The test starts 4 compactions with a slow iterator so there's plenty of time to have metrics reported.
When the test is running, you can use jconsole to examine jmx - or use the metric file output - with this the should be multiple metrics measurements. You may need to adjust the metrics config so that the metrics files are written - they will end up in the target directory of the ITs.
----------------------------------------------------------------
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 pull request #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#issuecomment-720844642
Since this branch has been sitting for a while, locally I merged this branch to main and reran all ITs matching *Compaction*IT*. The test were all a success.
----------------------------------------------------------------
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] EdColeman commented on pull request #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#issuecomment-669311242
Looking at the metrics generated by CompactionExecutorsMetrics.
My initial concern was that there could be cardinality issues if the metrics names are unbounded - that does not seem to be an issue. Using the current code in your branch, I quickly added a delay in the FateConcurrencyIT test that forces a pause while a compaction FATE transaction is running so that the metrics system has time to write records to the test output file. The generated record for the IT test tserver looks like:
`1596645249079 tserver.compactionExecutors: Context=tserver, ProcessName=TabletServer, Hostname=ip-xxx-xxx-xxx-xxx, default_large_queued=0, default_large_running=0, default_medium_queued=0, default_medium_running=0, default_small_queued=0, default_small_running=0, meta_huge_queued=0, meta_huge_running=0, meta_small_queued=0, meta_small_running=0, root_small_queued=0, root_small_running=0, root_huge_queued=0, root_huge_running=0, default_any_queued=0, default_any_running=1`
This looks good, but why is default_any_running=1 but nothing in default large, medium or small? Is default_any a separate queue, or should it be the sum of all default_any_ ?
With this forced test, I know the FATE transaction is "IN_PROGRESS" - it uses a slow compaction runner that pauses 250ms in the iterator before each next() call - could that be changing the "running" to something where the thread is paused and that does not count as "running"
Before digging I just want to validate that my assumptions w.r.t the names is accurate.
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r455810772
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -415,6 +415,10 @@
TSERV_COMPACTION_SERVICE_ROOT_PLANNER("tserver.compaction.major.service.root.planner",
DefaultCompactionPlanner.class.getName(), PropertyType.CLASSNAME,
"Compaction planner for root tablet service"),
+ TSERV_COMPACTION_SERVICE_ROOT_THROUGHPUT("tserver.compaction.major.service.root.throughput", "0B",
+ PropertyType.BYTES,
+ "Maximum number of bytes to read or write per second over all major"
+ + " compactions in this compaction service, or 0B for unlimited."),
Review comment:
I think a property that is easier to spell is more user friendly. The last part of the property could be "limit" or "rate.limit". As a bad speller, I know a word like "throughput" would trip me up.
##########
File path: test/src/main/java/org/apache/accumulo/test/CompactionRateLimitingIT.java
##########
@@ -37,50 +40,74 @@
public static final long BYTES_TO_WRITE = 10 * 1024 * 1024;
public static final long RATE = 1 * 1024 * 1024;
+ protected Property getThroughputProp() {
+ return Property.TSERV_COMPACTION_SERVICE_DEFAULT_THROUGHPUT;
+ }
+
@Override
public void configure(MiniAccumuloConfigImpl cfg, Configuration fsConf) {
- cfg.setProperty(Property.TSERV_MAJC_THROUGHPUT, RATE + "B");
+ cfg.setProperty(getThroughputProp(), RATE + "B");
cfg.setProperty(Property.TABLE_MAJC_RATIO, "20");
cfg.setProperty(Property.TABLE_FILE_COMPRESSION_TYPE, "none");
+
+ cfg.setProperty("tserver.compaction.major.service.test.throughput", RATE + "B");
+ cfg.setProperty("tserver.compaction.major.service.test.planner",
+ DefaultCompactionPlanner.class.getName());
+ cfg.setProperty("tserver.compaction.major.service.test.planner.opts.executors",
+ "[{'name':'all','numThreads':2}]".replaceAll("'", "\""));
+
}
@Test
public void majorCompactionsAreRateLimited() throws Exception {
long bytesWritten = 0;
- String tableName = getUniqueNames(1)[0];
- AccumuloClient client =
- getCluster().createAccumuloClient("root", new PasswordToken(ROOT_PASSWORD));
- client.tableOperations().create(tableName);
- try (BatchWriter bw = client.createBatchWriter(tableName)) {
- Random r = new SecureRandom();
- while (bytesWritten < BYTES_TO_WRITE) {
- byte[] rowKey = new byte[32];
- r.nextBytes(rowKey);
-
- byte[] qual = new byte[32];
- r.nextBytes(qual);
-
- byte[] value = new byte[1024];
- r.nextBytes(value);
-
- Mutation m = new Mutation(rowKey);
- m.put(new byte[0], qual, value);
- bw.addMutation(m);
-
- bytesWritten += rowKey.length + qual.length + value.length;
+ String[] tableNames = getUniqueNames(1);
+
+ try (AccumuloClient client =
+ getCluster().createAccumuloClient("root", new PasswordToken(ROOT_PASSWORD))) {
+
+ for (int i = 0; i < tableNames.length; i++) {
+ String tableName = tableNames[i];
+
+ NewTableConfiguration ntc = new NewTableConfiguration();
+ if (i == 1) {
+ ntc.setProperties(Map.of("table.compaction.dispatcher.opts.service", "test"));
+ }
+
+ client.tableOperations().create(tableName, ntc);
+ try (BatchWriter bw = client.createBatchWriter(tableName)) {
+ Random r = new SecureRandom();
+ while (bytesWritten < BYTES_TO_WRITE) {
+ byte[] rowKey = new byte[32];
+ r.nextBytes(rowKey);
+
+ byte[] qual = new byte[32];
+ r.nextBytes(qual);
+
+ byte[] value = new byte[1024];
+ r.nextBytes(value);
+
+ Mutation m = new Mutation(rowKey);
+ m.put(new byte[0], qual, value);
+ bw.addMutation(m);
+
+ bytesWritten += rowKey.length + qual.length + value.length;
+ }
+ }
+
+ client.tableOperations().flush(tableName, null, null, true);
+
+ long compactionStart = System.currentTimeMillis();
+ client.tableOperations().compact(tableName, null, null, false, true);
+ long duration = System.currentTimeMillis() - compactionStart;
+ // The rate will be "bursty", try to account for that by taking 80% of the expected rate
+ // (allow
+ // for 20% under the maximum expected duration)
Review comment:
Comment got awkward by the formatter.
----------------------------------------------------------------
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 #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r464480961
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -415,6 +415,10 @@
TSERV_COMPACTION_SERVICE_ROOT_PLANNER("tserver.compaction.major.service.root.planner",
DefaultCompactionPlanner.class.getName(), PropertyType.CLASSNAME,
"Compaction planner for root tablet service"),
+ TSERV_COMPACTION_SERVICE_ROOT_RATE_LIMIT("tserver.compaction.major.service.root.rate.limit", "0B",
Review comment:
The previous property that shares the same prefix mentions the root table.
----------------------------------------------------------------
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 #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r456741163
##########
File path: shell/src/main/java/org/apache/accumulo/shell/commands/CompactCommand.java
##########
@@ -147,30 +170,28 @@ public int execute(final String fullCommand, final CommandLine cl, final Shell s
compactionConfig.setIterators(new ArrayList<>(iterators));
}
- Map<String,String> selectorOpts = new HashMap<>();
- Map<String,String> configurerOpts = new HashMap<>();
- getConfigurableCompactionStrategyOpts(cl, selectorOpts, configurerOpts);
+ setupConfigurableCompaction(cl, compactionConfig);
if (cl.hasOption(strategyOpt.getOpt())) {
- if (!selectorOpts.isEmpty() || !configurerOpts.isEmpty())
+ if (cl.hasOption(selectorOpt.getLongOpt()) || cl.hasOption(configurerOpt.getLongOpt())) {
throw new IllegalArgumentException(
- "Can not specify compaction strategy with file selection and file output options.");
-
+ "Can not specify a strategy with a selector or configurer");
+ }
configureCompactionStrat(cl);
}
- if (!selectorOpts.isEmpty()) {
- PluginConfig selectorCfg = new PluginConfig(
- "org.apache.accumulo.tserver.compaction.strategies.ConfigurableCompactionStrategy",
- selectorOpts);
- compactionConfig.setSelector(selectorCfg);
+ if (cl.hasOption(selectorOpt.getLongOpt())) {
Review comment:
Fixed in abc961a
----------------------------------------------------------------
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] EdColeman commented on a change in pull request #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r465997384
##########
File path: test/src/main/java/org/apache/accumulo/test/CompactionRateLimitingIT.java
##########
@@ -37,50 +40,73 @@
public static final long BYTES_TO_WRITE = 10 * 1024 * 1024;
public static final long RATE = 1 * 1024 * 1024;
+ protected Property getThroughputProp() {
+ return Property.TSERV_COMPACTION_SERVICE_DEFAULT_RATE_LIMIT;
+ }
+
@Override
public void configure(MiniAccumuloConfigImpl cfg, Configuration fsConf) {
- cfg.setProperty(Property.TSERV_MAJC_THROUGHPUT, RATE + "B");
+ cfg.setProperty(getThroughputProp(), RATE + "B");
cfg.setProperty(Property.TABLE_MAJC_RATIO, "20");
cfg.setProperty(Property.TABLE_FILE_COMPRESSION_TYPE, "none");
+
+ cfg.setProperty("tserver.compaction.major.service.test.rate.limit", RATE + "B");
+ cfg.setProperty("tserver.compaction.major.service.test.planner",
+ DefaultCompactionPlanner.class.getName());
+ cfg.setProperty("tserver.compaction.major.service.test.planner.opts.executors",
+ "[{'name':'all','numThreads':2}]".replaceAll("'", "\""));
+
}
@Test
public void majorCompactionsAreRateLimited() throws Exception {
long bytesWritten = 0;
- String tableName = getUniqueNames(1)[0];
- AccumuloClient client =
- getCluster().createAccumuloClient("root", new PasswordToken(ROOT_PASSWORD));
- client.tableOperations().create(tableName);
- try (BatchWriter bw = client.createBatchWriter(tableName)) {
- Random r = new SecureRandom();
- while (bytesWritten < BYTES_TO_WRITE) {
- byte[] rowKey = new byte[32];
- r.nextBytes(rowKey);
-
- byte[] qual = new byte[32];
- r.nextBytes(qual);
-
- byte[] value = new byte[1024];
- r.nextBytes(value);
-
- Mutation m = new Mutation(rowKey);
- m.put(new byte[0], qual, value);
- bw.addMutation(m);
-
- bytesWritten += rowKey.length + qual.length + value.length;
+ String[] tableNames = getUniqueNames(1);
+
+ try (AccumuloClient client =
+ getCluster().createAccumuloClient("root", new PasswordToken(ROOT_PASSWORD))) {
+
+ for (int i = 0; i < tableNames.length; i++) {
+ String tableName = tableNames[i];
+
+ NewTableConfiguration ntc = new NewTableConfiguration();
+ if (i == 1) {
+ ntc.setProperties(Map.of("table.compaction.dispatcher.opts.service", "test"));
+ }
+
+ client.tableOperations().create(tableName, ntc);
+ try (BatchWriter bw = client.createBatchWriter(tableName)) {
+ Random r = new SecureRandom();
+ while (bytesWritten < BYTES_TO_WRITE) {
+ byte[] rowKey = new byte[32];
+ r.nextBytes(rowKey);
+
+ byte[] qual = new byte[32];
+ r.nextBytes(qual);
+
+ byte[] value = new byte[1024];
+ r.nextBytes(value);
+
+ Mutation m = new Mutation(rowKey);
+ m.put(new byte[0], qual, value);
+ bw.addMutation(m);
+
+ bytesWritten += rowKey.length + qual.length + value.length;
+ }
+ }
+
+ client.tableOperations().flush(tableName, null, null, true);
+
+ long compactionStart = System.currentTimeMillis();
+ client.tableOperations().compact(tableName, null, null, false, true);
+ long duration = System.currentTimeMillis() - compactionStart;
+ // The rate will be "bursty", try to account for that by taking 80% of the expected rate
+ // (allow for 20% under the maximum expected duration)
+ assertTrue(String.format(
+ "Expected a compaction rate of no more than %,d bytes/sec, but saw a rate of %,f bytes/sec",
+ (int) 0.8d * RATE, 1000.0 * bytesWritten / duration),
Review comment:
```suggestion
(int) (0.8d * RATE), 1000.0 * bytesWritten / duration),
```
----------------------------------------------------------------
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 #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r464485329
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionExecutor.java
##########
@@ -94,8 +112,11 @@ public boolean cancel(Status expectedStatus) {
canceled = status.compareAndSet(expectedStatus, Status.CANCELED);
}
+ if (canceled)
+ queuedTask.remove(this);
+
if (canceled && cancelCount.incrementAndGet() % 1024 == 0) {
- // nMeed to occasionally clean the queue, it could have canceled task with low priority that
+ // need to occasionally clean the queue, it could have canceled task with low priority that
// hang around. Avoid cleaning it every time something is canceled as that could be
// expensive.
Review comment:
Awesome, thanks! A minor cleanup would be great!
----------------------------------------------------------------
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 pull request #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#issuecomment-713876034
@EdColeman @milleruntime @ctubbsii I would like to merge this before it ends up w/ conflicts. I think I addressed all comments. Did I miss anything or where there any other questions?
----------------------------------------------------------------
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 #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r456741075
##########
File path: test/src/main/java/org/apache/accumulo/test/CompactionRateLimitingIT.java
##########
@@ -37,50 +40,74 @@
public static final long BYTES_TO_WRITE = 10 * 1024 * 1024;
public static final long RATE = 1 * 1024 * 1024;
+ protected Property getThroughputProp() {
+ return Property.TSERV_COMPACTION_SERVICE_DEFAULT_THROUGHPUT;
+ }
+
@Override
public void configure(MiniAccumuloConfigImpl cfg, Configuration fsConf) {
- cfg.setProperty(Property.TSERV_MAJC_THROUGHPUT, RATE + "B");
+ cfg.setProperty(getThroughputProp(), RATE + "B");
cfg.setProperty(Property.TABLE_MAJC_RATIO, "20");
cfg.setProperty(Property.TABLE_FILE_COMPRESSION_TYPE, "none");
+
+ cfg.setProperty("tserver.compaction.major.service.test.throughput", RATE + "B");
+ cfg.setProperty("tserver.compaction.major.service.test.planner",
+ DefaultCompactionPlanner.class.getName());
+ cfg.setProperty("tserver.compaction.major.service.test.planner.opts.executors",
+ "[{'name':'all','numThreads':2}]".replaceAll("'", "\""));
+
}
@Test
public void majorCompactionsAreRateLimited() throws Exception {
long bytesWritten = 0;
- String tableName = getUniqueNames(1)[0];
- AccumuloClient client =
- getCluster().createAccumuloClient("root", new PasswordToken(ROOT_PASSWORD));
- client.tableOperations().create(tableName);
- try (BatchWriter bw = client.createBatchWriter(tableName)) {
- Random r = new SecureRandom();
- while (bytesWritten < BYTES_TO_WRITE) {
- byte[] rowKey = new byte[32];
- r.nextBytes(rowKey);
-
- byte[] qual = new byte[32];
- r.nextBytes(qual);
-
- byte[] value = new byte[1024];
- r.nextBytes(value);
-
- Mutation m = new Mutation(rowKey);
- m.put(new byte[0], qual, value);
- bw.addMutation(m);
-
- bytesWritten += rowKey.length + qual.length + value.length;
+ String[] tableNames = getUniqueNames(1);
+
+ try (AccumuloClient client =
+ getCluster().createAccumuloClient("root", new PasswordToken(ROOT_PASSWORD))) {
+
+ for (int i = 0; i < tableNames.length; i++) {
+ String tableName = tableNames[i];
+
+ NewTableConfiguration ntc = new NewTableConfiguration();
+ if (i == 1) {
+ ntc.setProperties(Map.of("table.compaction.dispatcher.opts.service", "test"));
+ }
+
+ client.tableOperations().create(tableName, ntc);
+ try (BatchWriter bw = client.createBatchWriter(tableName)) {
+ Random r = new SecureRandom();
+ while (bytesWritten < BYTES_TO_WRITE) {
+ byte[] rowKey = new byte[32];
+ r.nextBytes(rowKey);
+
+ byte[] qual = new byte[32];
+ r.nextBytes(qual);
+
+ byte[] value = new byte[1024];
+ r.nextBytes(value);
+
+ Mutation m = new Mutation(rowKey);
+ m.put(new byte[0], qual, value);
+ bw.addMutation(m);
+
+ bytesWritten += rowKey.length + qual.length + value.length;
+ }
+ }
+
+ client.tableOperations().flush(tableName, null, null, true);
+
+ long compactionStart = System.currentTimeMillis();
+ client.tableOperations().compact(tableName, null, null, false, true);
+ long duration = System.currentTimeMillis() - compactionStart;
+ // The rate will be "bursty", try to account for that by taking 80% of the expected rate
+ // (allow
+ // for 20% under the maximum expected duration)
Review comment:
fixed in abc961a
----------------------------------------------------------------
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 #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r464480153
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionExecutor.java
##########
@@ -94,8 +112,11 @@ public boolean cancel(Status expectedStatus) {
canceled = status.compareAndSet(expectedStatus, Status.CANCELED);
}
+ if (canceled)
+ queuedTask.remove(this);
+
if (canceled && cancelCount.incrementAndGet() % 1024 == 0) {
- // nMeed to occasionally clean the queue, it could have canceled task with low priority that
+ // need to occasionally clean the queue, it could have canceled task with low priority that
// hang around. Avoid cleaning it every time something is canceled as that could be
// expensive.
Review comment:
Changed comment in 79611ee
----------------------------------------------------------------
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 #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner merged pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649
----------------------------------------------------------------
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 #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r462571887
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -415,6 +415,10 @@
TSERV_COMPACTION_SERVICE_ROOT_PLANNER("tserver.compaction.major.service.root.planner",
DefaultCompactionPlanner.class.getName(), PropertyType.CLASSNAME,
"Compaction planner for root tablet service"),
+ TSERV_COMPACTION_SERVICE_ROOT_RATE_LIMIT("tserver.compaction.major.service.root.rate.limit", "0B",
Review comment:
Also, what does "root" refer to here for this property? The description should leave that obvious, rather than require users to refer to the descriptions of other properties that have similar naming schemes in order to guess at what it might mean for this property.
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionExecutor.java
##########
@@ -94,8 +112,11 @@ public boolean cancel(Status expectedStatus) {
canceled = status.compareAndSet(expectedStatus, Status.CANCELED);
}
+ if (canceled)
+ queuedTask.remove(this);
+
if (canceled && cancelCount.incrementAndGet() % 1024 == 0) {
- // nMeed to occasionally clean the queue, it could have canceled task with low priority that
+ // need to occasionally clean the queue, it could have canceled task with low priority that
// hang around. Avoid cleaning it every time something is canceled as that could be
// expensive.
Review comment:
This sentence could be cleaned up to make it more understandable. What does "it" refer to? And does it mean it could "contain a canceled task", or that it could "have canceled a task"?
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -415,6 +415,10 @@
TSERV_COMPACTION_SERVICE_ROOT_PLANNER("tserver.compaction.major.service.root.planner",
DefaultCompactionPlanner.class.getName(), PropertyType.CLASSNAME,
"Compaction planner for root tablet service"),
+ TSERV_COMPACTION_SERVICE_ROOT_RATE_LIMIT("tserver.compaction.major.service.root.rate.limit", "0B",
Review comment:
I worry that we have added *far* too many new configuration properties for compactions, and that this might create a terrible user experience around compactions. I wonder if some of these could be consolidated into a pluggable configuration strategy object that is less complicated to configure.
----------------------------------------------------------------
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] EdColeman commented on pull request #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
EdColeman commented on pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#issuecomment-713905190
From what I had looked at, you answered the concerns that I had. It seemed like you were going to try to manually examine the metrics emitted, do you have a chance? More out of curiosity, I'm not suggesting that it needs to hold this up.
----------------------------------------------------------------
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 #1649: Addresses four compaction service follow on issues.
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1649:
URL: https://github.com/apache/accumulo/pull/1649#discussion_r464470854
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -415,6 +415,10 @@
TSERV_COMPACTION_SERVICE_ROOT_PLANNER("tserver.compaction.major.service.root.planner",
DefaultCompactionPlanner.class.getName(), PropertyType.CLASSNAME,
"Compaction planner for root tablet service"),
+ TSERV_COMPACTION_SERVICE_ROOT_RATE_LIMIT("tserver.compaction.major.service.root.rate.limit", "0B",
Review comment:
> I worry that we have added far too many new configuration properties for compactions, and that this might create a terrible user experience around compactions.
Could open a new issue and discuss a course of action.
> Also, what does "root" refer to here for this property?
This is a compaction service for the root table.
----------------------------------------------------------------
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