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/05/08 01:12:53 UTC
[GitHub] [accumulo] keith-turner opened a new pull request #1605: Fixes #564 adds support multiple compaction executors
keith-turner opened a new pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605
This change adds support for multiple compaction executors and
multuple concurrent compactions per tablet.
The best way to understand these changes is to look at the documentation
at core/src/main/java/org/apache/accumulo/core/spi/compaction/package-info.java
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r441914291
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/CompactionConfig.java
##########
@@ -21,25 +21,37 @@
import static java.util.Objects.requireNonNull;
import static org.apache.accumulo.core.clientImpl.CompactionStrategyConfigUtil.DEFAULT_STRATEGY;
-import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
+import java.util.Map;
+import java.util.function.BooleanSupplier;
import org.apache.accumulo.core.client.IteratorSetting;
+import org.apache.accumulo.core.client.admin.compaction.CompactionConfigurer;
+import org.apache.accumulo.core.client.admin.compaction.CompactionSelector;
+import org.apache.accumulo.core.clientImpl.CompactionStrategyConfigUtil;
+import org.apache.accumulo.core.clientImpl.UserCompactionUtils;
import org.apache.hadoop.io.Text;
+import com.google.common.base.Preconditions;
+
/**
* This class exist to pass parameters to {@link TableOperations#compact(String, CompactionConfig)}
*
* @since 1.7.0
*/
public class CompactionConfig {
+
private Text start = null;
private Text end = null;
private boolean flush = true;
private boolean wait = true;
private List<IteratorSetting> iterators = Collections.emptyList();
+ @SuppressWarnings("removal")
private CompactionStrategyConfig compactionStrategy = DEFAULT_STRATEGY;
+ private Map<String,String> hints = Map.of();
+ private PluginConfig selectorConfig = UserCompactionUtils.DEFAULT_CSC;
+ private PluginConfig configurerConfig = UserCompactionUtils.DEFAULT_CCC;
Review comment:
done in 5aa9672
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422366764
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/PluginConfig.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * Encapsulates the configuration of an Accumulo server side plugin, which consist of a class name
+ * and options.
+ *
+ * @since 2.1.0
+ */
+public class PluginConfig {
+
+ private String className;
+ private Map<String,String> options = Collections.emptyMap();
+
+ /**
+ * @param className
+ * The name of a class that implements
+ * org.apache.accumulo.tserver.compaction.CompactionStrategy. This class must be exist on
+ * tservers.
Review comment:
fixed in ecc79e3
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422260699
##########
File path: assemble/conf/log4j2-service.properties
##########
@@ -78,6 +78,9 @@ logger.zookeeper.level = error
logger.accumulo.name = org.apache.accumulo
logger.accumulo.level = debug
+logger.audit.name = org.apache.accumulo.audit
+logger.audit.level = off
+
Review comment:
I don't know, it was probably annoying me as I spent a lot of time looking at the logs while working on this. That change should not be made in this PR, I will pull it out.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422366322
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/PluginConfig.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * Encapsulates the configuration of an Accumulo server side plugin, which consist of a class name
+ * and options.
+ *
+ * @since 2.1.0
+ */
+public class PluginConfig {
+
+ private String className;
+ private Map<String,String> options = Collections.emptyMap();
+
+ /**
+ * @param className
+ * The name of a class that implements
+ * org.apache.accumulo.tserver.compaction.CompactionStrategy. This class must be exist on
+ * tservers.
+ */
+ public PluginConfig(String className) {
+ requireNonNull(className);
+ this.className = className;
+ }
+
+ /**
+ * @return the class name passed to the constructor.
+ */
+ public String getClassName() {
+ return className;
+ }
+
+ /**
+ * @param opts
+ * The options that will be passed to the init() method of the plugin when its
+ * instantiated on a tserver. This method will copy the map. The default is an empty map.
+ * @return this
+ */
+ public PluginConfig setOptions(Map<String,String> opts) {
+ requireNonNull(opts);
+ this.options = Map.copyOf(opts);
+ return this;
Review comment:
fixed in ecc79e3
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422276492
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/CompactionStrategyConfig.java
##########
@@ -30,27 +26,28 @@
* {@link CompactionConfig}.
*
* @since 1.7.0
+ * @deprecated since 2.1.0 CompactionStrategies were deprecated for multiple reasons. First, they do
+ * not support the new compaction execution model. Second, they bind selection and
+ * output file configuration into a single entity when users need to configure these
+ * independently. Third, they use internal Accumulo types and ensuring their stability
Review comment:
My motivation was in the past we had two compaction strategies included with Accumulo, one that filtered compaction candidates based on max size and another that changed the compression type based on input files sizes. These had nothing to do with each other, but only one could be configured at a time.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#issuecomment-625972278
@ctubbsii something else that I eventually need to write is user facing documention. The documentation written so for is a bit more internal. The old WIP PR #1589 does have a bit of user facing documentation in its opening comment that would be useful to look at though. It shows how to set configure different compaction services and make tables use them. The properties have changed a bit, but all of the concepts are the same.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r441914459
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/CompactionConfig.java
##########
@@ -21,25 +21,37 @@
import static java.util.Objects.requireNonNull;
import static org.apache.accumulo.core.clientImpl.CompactionStrategyConfigUtil.DEFAULT_STRATEGY;
-import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
+import java.util.Map;
+import java.util.function.BooleanSupplier;
import org.apache.accumulo.core.client.IteratorSetting;
+import org.apache.accumulo.core.client.admin.compaction.CompactionConfigurer;
+import org.apache.accumulo.core.client.admin.compaction.CompactionSelector;
+import org.apache.accumulo.core.clientImpl.CompactionStrategyConfigUtil;
+import org.apache.accumulo.core.clientImpl.UserCompactionUtils;
import org.apache.hadoop.io.Text;
+import com.google.common.base.Preconditions;
+
/**
* This class exist to pass parameters to {@link TableOperations#compact(String, CompactionConfig)}
*
* @since 1.7.0
*/
public class CompactionConfig {
+
private Text start = null;
private Text end = null;
private boolean flush = true;
private boolean wait = true;
private List<IteratorSetting> iterators = Collections.emptyList();
+ @SuppressWarnings("removal")
private CompactionStrategyConfig compactionStrategy = DEFAULT_STRATEGY;
+ private Map<String,String> hints = Map.of();
+ private PluginConfig selectorConfig = UserCompactionUtils.DEFAULT_CSC;
Review comment:
done in 5aa9672
##########
File path: core/src/main/java/org/apache/accumulo/core/spi/compaction/CompactionServices.java
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.spi.compaction;
+
+import java.util.Set;
+
+/**
+ * @since 2.1.0
+ */
+public interface CompactionServices {
Review comment:
done in 5aa9672
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422289919
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/PluginConfig.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * Encapsulates the configuration of an Accumulo server side plugin, which consist of a class name
+ * and options.
+ *
+ * @since 2.1.0
+ */
+public class PluginConfig {
+
+ private String className;
+ private Map<String,String> options = Collections.emptyMap();
+
+ /**
+ * @param className
+ * The name of a class that implements
+ * org.apache.accumulo.tserver.compaction.CompactionStrategy. This class must be exist on
+ * tservers.
Review comment:
That should not be mentioned, that is a copy and paste bug. I will fix that.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422365160
##########
File path: assemble/conf/log4j2-service.properties
##########
@@ -78,6 +78,9 @@ logger.zookeeper.level = error
logger.accumulo.name = org.apache.accumulo
logger.accumulo.level = debug
+logger.audit.name = org.apache.accumulo.audit
+logger.audit.level = off
+
Review comment:
removed in ecc79e3
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422262167
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/CompactionStrategyConfigUtil.java
##########
@@ -18,23 +18,17 @@
*/
package org.apache.accumulo.core.clientImpl;
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
import java.io.DataInput;
-import java.io.DataInputStream;
import java.io.DataOutput;
-import java.io.DataOutputStream;
-import java.io.IOException;
-import java.util.HashMap;
import java.util.Map;
-import java.util.Map.Entry;
+import org.apache.accumulo.core.client.admin.CompactionConfig;
import org.apache.accumulo.core.client.admin.CompactionStrategyConfig;
+@SuppressWarnings("removal")
Review comment:
This entire class is utility code for a deprecated class.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner edited a comment on pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#issuecomment-643514856
There have not been any comments on this in a while, does anyone have any objections to me merging this? In other branches in my fork I have been working on following work for #1609, #1611, and #1612. I would like to merge this so I can submit PRs for those, which are much smaller relative to this.
I have run a test of this code on a cluster and plan to run more test. I would like to wait to run subsequent test after finishing the follow on work.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422366630
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/PluginConfig.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * Encapsulates the configuration of an Accumulo server side plugin, which consist of a class name
+ * and options.
+ *
+ * @since 2.1.0
+ */
+public class PluginConfig {
+
+ private String className;
+ private Map<String,String> options = Collections.emptyMap();
+
+ /**
+ * @param className
+ * The name of a class that implements
+ * org.apache.accumulo.tserver.compaction.CompactionStrategy. This class must be exist on
+ * tservers.
+ */
+ public PluginConfig(String className) {
+ requireNonNull(className);
+ this.className = className;
Review comment:
fixed in ecc79e3
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422350186
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/CompactionStrategyConfigUtil.java
##########
@@ -18,23 +18,17 @@
*/
package org.apache.accumulo.core.clientImpl;
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
import java.io.DataInput;
-import java.io.DataInputStream;
import java.io.DataOutput;
-import java.io.DataOutputStream;
-import java.io.IOException;
-import java.util.HashMap;
import java.util.Map;
-import java.util.Map.Entry;
+import org.apache.accumulo.core.client.admin.CompactionConfig;
import org.apache.accumulo.core.client.admin.CompactionStrategyConfig;
+@SuppressWarnings("removal")
Review comment:
I think other non-deprecated code calls this to deal with compaction strategies. I was trying to put the code that does this in one place and in order to avoid suppression elsewhere.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r421931808
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/CompactionStrategyConfig.java
##########
@@ -30,27 +26,28 @@
* {@link CompactionConfig}.
*
* @since 1.7.0
+ * @deprecated since 2.1.0 CompactionStrategies were deprecated for multiple reasons. First, they do
+ * not support the new compaction execution model. Second, they bind selection and
+ * output file configuration into a single entity when users need to configure these
+ * independently. Third, they use internal Accumulo types and ensuring their stability
Review comment:
I don't necessarily see the second reason to be a problem. Binding these into a single entity, can be nice for modularizing the user code to make it more reusable, and maintainable in a separate user-controlled repo. Under the new paradigm, what's the best way for users to combine their overall compaction strategy so they can maintain it separately, and just drop it in when needed? Would they just serialize the `CompactionConfig` in some way?
##########
File path: assemble/conf/log4j2-service.properties
##########
@@ -78,6 +78,9 @@ logger.zookeeper.level = error
logger.accumulo.name = org.apache.accumulo
logger.accumulo.level = debug
+logger.audit.name = org.apache.accumulo.audit
+logger.audit.level = off
+
Review comment:
Why disable these from the full logger by default?
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/PluginConfig.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * Encapsulates the configuration of an Accumulo server side plugin, which consist of a class name
+ * and options.
+ *
+ * @since 2.1.0
+ */
+public class PluginConfig {
+
+ private String className;
+ private Map<String,String> options = Collections.emptyMap();
+
+ /**
+ * @param className
+ * The name of a class that implements
+ * org.apache.accumulo.tserver.compaction.CompactionStrategy. This class must be exist on
+ * tservers.
+ */
+ public PluginConfig(String className) {
+ requireNonNull(className);
+ this.className = className;
Review comment:
```suggestion
this.className = requireNonNull(className);
```
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/PluginConfig.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * Encapsulates the configuration of an Accumulo server side plugin, which consist of a class name
+ * and options.
+ *
+ * @since 2.1.0
+ */
+public class PluginConfig {
+
+ private String className;
+ private Map<String,String> options = Collections.emptyMap();
+
+ /**
+ * @param className
+ * The name of a class that implements
+ * org.apache.accumulo.tserver.compaction.CompactionStrategy. This class must be exist on
+ * tservers.
Review comment:
Should the CompactionStrategy interface exist in an SPI package? Otherwise, it seems we haven't completely solved the use of internal types here.
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/PluginConfig.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * Encapsulates the configuration of an Accumulo server side plugin, which consist of a class name
+ * and options.
+ *
+ * @since 2.1.0
+ */
+public class PluginConfig {
+
+ private String className;
+ private Map<String,String> options = Collections.emptyMap();
+
+ /**
+ * @param className
+ * The name of a class that implements
+ * org.apache.accumulo.tserver.compaction.CompactionStrategy. This class must be exist on
+ * tservers.
+ */
+ public PluginConfig(String className) {
+ requireNonNull(className);
+ this.className = className;
+ }
+
+ /**
+ * @return the class name passed to the constructor.
+ */
+ public String getClassName() {
+ return className;
+ }
+
+ /**
+ * @param opts
+ * The options that will be passed to the init() method of the plugin when its
+ * instantiated on a tserver. This method will copy the map. The default is an empty map.
+ * @return this
+ */
+ public PluginConfig setOptions(Map<String,String> opts) {
+ requireNonNull(opts);
+ this.options = Map.copyOf(opts);
+ return this;
Review comment:
```suggestion
this.options = Map.copyOf(requireNonNull(opts));
return this;
```
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/compaction/CompactionConfigurer.java
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin.compaction;
+
+import java.util.Collection;
+import java.util.Map;
+
+import org.apache.accumulo.core.client.PluginEnvironment;
+import org.apache.accumulo.core.data.TableId;
+
+/**
+ * Enables dynamically overriding of per table properties used to create the output file for a
+ * compaction. For example it could override the per table property for compression.
+ *
+ * @since 2.1.0
+ */
+public interface CompactionConfigurer {
Review comment:
I'm confused by this javadoc. I don't think this javadoc is sufficient for me to understand how it is intended to be used, without additional outside documentation. Can you provide accompanying code for the compression example?
Also, I'm wondering if we can come up with a better name for this. I might be able to think of something once I get a better grasp on how it's supposed to be used.
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/PluginConfig.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * Encapsulates the configuration of an Accumulo server side plugin, which consist of a class name
+ * and options.
+ *
+ * @since 2.1.0
+ */
+public class PluginConfig {
+
+ private String className;
+ private Map<String,String> options = Collections.emptyMap();
+
+ /**
+ * @param className
+ * The name of a class that implements
+ * org.apache.accumulo.tserver.compaction.CompactionStrategy. This class must be exist on
+ * tservers.
+ */
+ public PluginConfig(String className) {
+ requireNonNull(className);
+ this.className = className;
+ }
+
+ /**
+ * @return the class name passed to the constructor.
+ */
+ public String getClassName() {
+ return className;
+ }
+
+ /**
+ * @param opts
+ * The options that will be passed to the init() method of the plugin when its
+ * instantiated on a tserver. This method will copy the map. The default is an empty map.
+ * @return this
+ */
+ public PluginConfig setOptions(Map<String,String> opts) {
+ requireNonNull(opts);
+ this.options = Map.copyOf(opts);
+ return this;
+ }
+
+ /**
+ * @return The previously set options. Returns an unmodifiable map. The default is an empty map.
+ */
+ public Map<String,String> getOptions() {
+ return options;
+ }
+
+ @Override
+ public int hashCode() {
+ return className.hashCode() + options.hashCode();
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o instanceof PluginConfig) {
+ PluginConfig ocsc = (PluginConfig) o;
+ return className.equals(ocsc.className) && options.equals(ocsc.options);
+ }
Review comment:
Since `className` and `options` have setters, this class is mutable, and changing it can result in unpredictable behavior if stored in data structures that use `hashCode` and `equals`. Can this class be refactored to be immutable?
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/UserCompactionUtils.java
##########
@@ -0,0 +1,301 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.clientImpl;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.DataInput;
+import java.io.DataInputStream;
+import java.io.DataOutput;
+import java.io.DataOutputStream;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Map.Entry;
+
+import org.apache.accumulo.core.client.IteratorSetting;
+import org.apache.accumulo.core.client.admin.CompactionConfig;
+import org.apache.accumulo.core.client.admin.PluginConfig;
+import org.apache.hadoop.io.Text;
+
+import com.google.common.base.Preconditions;
+
+public class UserCompactionUtils {
+
+ private static final int MAGIC = 0x02040810;
+ private static final int SELECTOR_MAGIC = 0xae9270bf;
+ private static final int CONFIGURER_MAGIC = 0xf93e570a;
Review comment:
Where do these numbers come from? An inline comment would be useful.
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/CompactionStrategyConfigUtil.java
##########
@@ -18,23 +18,17 @@
*/
package org.apache.accumulo.core.clientImpl;
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
import java.io.DataInput;
-import java.io.DataInputStream;
import java.io.DataOutput;
-import java.io.DataOutputStream;
-import java.io.IOException;
-import java.util.HashMap;
import java.util.Map;
-import java.util.Map.Entry;
+import org.apache.accumulo.core.client.admin.CompactionConfig;
import org.apache.accumulo.core.client.admin.CompactionStrategyConfig;
+@SuppressWarnings("removal")
Review comment:
Can this suppression occur more narrowly, rather than for the entire class?
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -410,6 +408,49 @@
"The number of threads for the metadata table scan executor."),
TSERV_MIGRATE_MAXCONCURRENT("tserver.migrations.concurrent.max", "1", PropertyType.COUNT,
"The maximum number of concurrent tablet migrations for a tablet server"),
+ TSERV_MAJC_DELAY("tserver.compaction.major.delay", "30s", PropertyType.TIMEDURATION,
+ "Time a tablet server will sleep between checking which tablets need compaction."),
+ TSERV_COMPACTION_SERVICE_PREFIX("tserver.compaction.service.", null, PropertyType.PREFIX,
Review comment:
Presumably, these new properties are for major compactions only. The property prefix could be made consistent with other `tserver.compaction.major` properties.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r439649889
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -410,6 +408,49 @@
"The number of threads for the metadata table scan executor."),
TSERV_MIGRATE_MAXCONCURRENT("tserver.migrations.concurrent.max", "1", PropertyType.COUNT,
"The maximum number of concurrent tablet migrations for a tablet server"),
+ TSERV_MAJC_DELAY("tserver.compaction.major.delay", "30s", PropertyType.TIMEDURATION,
+ "Time a tablet server will sleep between checking which tablets need compaction."),
+ TSERV_COMPACTION_SERVICE_PREFIX("tserver.compaction.service.", null, PropertyType.PREFIX,
Review comment:
Done in 8d024d8
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#issuecomment-643514856
There have not been any comments on this in a while, does anyone have any objections to me merging this? In other branches in my fork I have been working on following work for #1609, #1611, and #1612. I would like to merge this so I can submit PRs for those, which are much smaller relative to 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] keith-turner commented on a change in pull request #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422268789
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/compaction/CompactionConfigurer.java
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin.compaction;
+
+import java.util.Collection;
+import java.util.Map;
+
+import org.apache.accumulo.core.client.PluginEnvironment;
+import org.apache.accumulo.core.data.TableId;
+
+/**
+ * Enables dynamically overriding of per table properties used to create the output file for a
+ * compaction. For example it could override the per table property for compression.
+ *
+ * @since 2.1.0
+ */
+public interface CompactionConfigurer {
Review comment:
[CompressionConfigurer](https://github.com/apache/accumulo/blob/bd206be8ed50ebf1fee76dc8cf1e9820ef8ca5b8/core/src/main/java/org/apache/accumulo/core/client/admin/compaction/CompressionConfigurer.java) is an example. It overrides the table property for compression type when the sum of input files exceeds a certain size.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner merged pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#issuecomment-644416350
@milleruntime I found all the TODOs and addressed/removed them.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422347761
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/CompactionStrategyConfigUtil.java
##########
@@ -18,23 +18,17 @@
*/
package org.apache.accumulo.core.clientImpl;
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
import java.io.DataInput;
-import java.io.DataInputStream;
import java.io.DataOutput;
-import java.io.DataOutputStream;
-import java.io.IOException;
-import java.util.HashMap;
import java.util.Map;
-import java.util.Map.Entry;
+import org.apache.accumulo.core.client.admin.CompactionConfig;
import org.apache.accumulo.core.client.admin.CompactionStrategyConfig;
+@SuppressWarnings("removal")
Review comment:
In that case, it too can be marked `@Deprecated` instead of merely suppressing the deprecation.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r441914581
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/EverythingCompactionStrategy.java
##########
@@ -22,7 +22,7 @@
* The default compaction strategy for user initiated compactions. This strategy will always select
* all files.
*/
-
+@SuppressWarnings("removal")
Review comment:
good catch, fixed in 5aa9672
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r441915358
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java
##########
@@ -239,12 +238,7 @@ public static StoredTabletFile updateTabletDataFile(ServerContext context, KeyEx
}
tablet.putFlushId(flushId);
- if (mergeFile != null) {
- tablet.deleteFile(mergeFile);
- }
-
unusedWalLogs.forEach(tablet::deleteWal);
- filesInUseByScans.forEach(tablet::putScan);
Review comment:
yeah, eliminating merging minor compaction eliminated the need of dealing with a file being merged and in use by scans.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#issuecomment-625944170
> Would there be value in marking the new properties/APIs as "experimental" until it bears out after a release or two?
That would entail keeping all the old code and then jamming this new code into the old code. I don't think that would help achieve stability or maintainability. There are actually a lot of big changes that have built up for 2.1.0. I think we need to stop accepting new features for 2.1.0 at some point and start heavily testing it and only fixing bugs.
> There are so many new classes. The number of new classes might make the compaction code a bit more logical, but it's a big shift from the current code, and very hard for a newcomer to quickly familiarize themselves with all the code.
Did you look at the javadoc link I posted? The current plugins break down by data and execution. The CompactionSelector, CompactionConfigurer, and iterators are only concerned with user data and can be set per table or per compaction. The CompactionPlanner and CompactionDispatcher are only concerned with executing compactions. Planners are configured at the system level and dispatchers are configured per table. Users can set execution hints when initiating a compaction through the API and these hints are made available to the dispatcher and planner. If this is helpful, I can work this into the javadoc.
> It might help to organize a video chat or slide show to go over all the code changes at a high level, so that other Accumulo devs can quickly assess the new design.
I can do a chat on slack. One goal of that could be to determine how the written documentation could be improved.
> I kind of liked the idea of setting a single compaction strategy property that encapsulates all the configuration
This is very vague and I don't know what this would concretely look like. We could discuss this in the chat. If you have more specific ideas, it would helpful to write them up somewhere.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422262625
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/UserCompactionUtils.java
##########
@@ -0,0 +1,301 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.clientImpl;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.DataInput;
+import java.io.DataInputStream;
+import java.io.DataOutput;
+import java.io.DataOutputStream;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Map.Entry;
+
+import org.apache.accumulo.core.client.IteratorSetting;
+import org.apache.accumulo.core.client.admin.CompactionConfig;
+import org.apache.accumulo.core.client.admin.PluginConfig;
+import org.apache.hadoop.io.Text;
+
+import com.google.common.base.Preconditions;
+
+public class UserCompactionUtils {
+
+ private static final int MAGIC = 0x02040810;
+ private static final int SELECTOR_MAGIC = 0xae9270bf;
+ private static final int CONFIGURER_MAGIC = 0xf93e570a;
Review comment:
I don't remember. I may have taken a git commit hash or they may be random. The intent is to be random.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422293917
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/PluginConfig.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * Encapsulates the configuration of an Accumulo server side plugin, which consist of a class name
+ * and options.
+ *
+ * @since 2.1.0
+ */
+public class PluginConfig {
+
+ private String className;
+ private Map<String,String> options = Collections.emptyMap();
+
+ /**
+ * @param className
+ * The name of a class that implements
+ * org.apache.accumulo.tserver.compaction.CompactionStrategy. This class must be exist on
+ * tservers.
+ */
+ public PluginConfig(String className) {
+ requireNonNull(className);
+ this.className = className;
+ }
+
+ /**
+ * @return the class name passed to the constructor.
+ */
+ public String getClassName() {
+ return className;
+ }
+
+ /**
+ * @param opts
+ * The options that will be passed to the init() method of the plugin when its
+ * instantiated on a tserver. This method will copy the map. The default is an empty map.
+ * @return this
+ */
+ public PluginConfig setOptions(Map<String,String> opts) {
+ requireNonNull(opts);
+ this.options = Map.copyOf(opts);
+ return this;
Review comment:
The javadoc for Map.copyOf says it will thow an NPE if its input is null. So the requireNonNull could be dropped.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r441058243
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/ActiveCompaction.java
##########
@@ -35,9 +35,9 @@
*/
MINOR,
/**
- * compaction to flush a tablets memory and merge it with the tablets smallest file. This type
- * compaction is done when a tablet has too many files
+ * Accumulo no longer does merging minor compactions.
Review comment:
Yes
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r433507305
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/compaction/CompactionConfigurer.java
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin.compaction;
+
+import java.util.Collection;
+import java.util.Map;
+
+import org.apache.accumulo.core.client.PluginEnvironment;
+import org.apache.accumulo.core.data.TableId;
+
+/**
+ * Enables dynamically overriding of per table properties used to create the output file for a
+ * compaction. For example it could override the per table property for compression.
+ *
+ * @since 2.1.0
+ */
+public interface CompactionConfigurer {
Review comment:
I agree the current name is not great. I am not too excited about CompressionConfigFactory either. One thing that occurred to me while running a recent test on a cluster of this PR is that in addition to thinking of a new class name, we also need to think about the table property names. Below are table props I set when running the test.
```
table.compaction.configurer=org.apache.accumulo.core.client.admin.compaction.CompressionConfigurer
table.compaction.configurer.opts.large.compress.threshold=100M
table.compaction.configurer.opts.large.compress.type=gz
```
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422365919
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/PluginConfig.java
##########
@@ -0,0 +1,94 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import static java.util.Objects.requireNonNull;
+
+import java.util.Collections;
+import java.util.Map;
+
+/**
+ * Encapsulates the configuration of an Accumulo server side plugin, which consist of a class name
+ * and options.
+ *
+ * @since 2.1.0
+ */
+public class PluginConfig {
+
+ private String className;
+ private Map<String,String> options = Collections.emptyMap();
+
+ /**
+ * @param className
+ * The name of a class that implements
+ * org.apache.accumulo.tserver.compaction.CompactionStrategy. This class must be exist on
+ * tservers.
+ */
+ public PluginConfig(String className) {
+ requireNonNull(className);
+ this.className = className;
+ }
+
+ /**
+ * @return the class name passed to the constructor.
+ */
+ public String getClassName() {
+ return className;
+ }
+
+ /**
+ * @param opts
+ * The options that will be passed to the init() method of the plugin when its
+ * instantiated on a tserver. This method will copy the map. The default is an empty map.
+ * @return this
+ */
+ public PluginConfig setOptions(Map<String,String> opts) {
+ requireNonNull(opts);
+ this.options = Map.copyOf(opts);
+ return this;
+ }
+
+ /**
+ * @return The previously set options. Returns an unmodifiable map. The default is an empty map.
+ */
+ public Map<String,String> getOptions() {
+ return options;
+ }
+
+ @Override
+ public int hashCode() {
+ return className.hashCode() + options.hashCode();
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o instanceof PluginConfig) {
+ PluginConfig ocsc = (PluginConfig) o;
+ return className.equals(ocsc.className) && options.equals(ocsc.options);
+ }
Review comment:
Made immutable in ecc79e3
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r433510222
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/CompactionStrategyConfig.java
##########
@@ -30,27 +26,28 @@
* {@link CompactionConfig}.
*
* @since 1.7.0
+ * @deprecated since 2.1.0 CompactionStrategies were deprecated for multiple reasons. First, they do
+ * not support the new compaction execution model. Second, they bind selection and
+ * output file configuration into a single entity when users need to configure these
+ * independently. Third, they use internal Accumulo types and ensuring their stability
Review comment:
@ctubbsii did the answer and the slack chat we had resolve this or do you think there is still something to be done?
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner edited a comment on pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#issuecomment-643514856
There have not been any comments on this in a while, does anyone have any objections to me merging this? In other branches in my fork I have been working on following work for #1609, #1611, and #1612. I would like to merge this so I can submit PRs for those, which are much smaller relative to this.
I have [run a test of this code](https://gist.github.com/keith-turner/29b72dfa97acad53c395dd0724b65f1c) on a cluster and plan to run more test. I would like to wait to run subsequent test after finishing the follow on work.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r424685598
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/compaction/CompactionConfigurer.java
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin.compaction;
+
+import java.util.Collection;
+import java.util.Map;
+
+import org.apache.accumulo.core.client.PluginEnvironment;
+import org.apache.accumulo.core.data.TableId;
+
+/**
+ * Enables dynamically overriding of per table properties used to create the output file for a
+ * compaction. For example it could override the per table property for compression.
+ *
+ * @since 2.1.0
+ */
+public interface CompactionConfigurer {
Review comment:
Would this be more appropriately named as a factory? It seems to be creating a compaction configuration - so maybe something like CompressionConfigFactory?
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r440813813
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/CompactionConfig.java
##########
@@ -21,25 +21,37 @@
import static java.util.Objects.requireNonNull;
import static org.apache.accumulo.core.clientImpl.CompactionStrategyConfigUtil.DEFAULT_STRATEGY;
-import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
+import java.util.Map;
+import java.util.function.BooleanSupplier;
import org.apache.accumulo.core.client.IteratorSetting;
+import org.apache.accumulo.core.client.admin.compaction.CompactionConfigurer;
+import org.apache.accumulo.core.client.admin.compaction.CompactionSelector;
+import org.apache.accumulo.core.clientImpl.CompactionStrategyConfigUtil;
+import org.apache.accumulo.core.clientImpl.UserCompactionUtils;
import org.apache.hadoop.io.Text;
+import com.google.common.base.Preconditions;
+
/**
* This class exist to pass parameters to {@link TableOperations#compact(String, CompactionConfig)}
*
* @since 1.7.0
*/
public class CompactionConfig {
+
private Text start = null;
private Text end = null;
private boolean flush = true;
private boolean wait = true;
private List<IteratorSetting> iterators = Collections.emptyList();
+ @SuppressWarnings("removal")
private CompactionStrategyConfig compactionStrategy = DEFAULT_STRATEGY;
+ private Map<String,String> hints = Map.of();
+ private PluginConfig selectorConfig = UserCompactionUtils.DEFAULT_CSC;
Review comment:
I think these names weren't updated after previous changes.
```suggestion
private PluginConfig selectorConfig = UserCompactionUtils.DEFAULT_SELECTOR;
```
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/EverythingCompactionStrategy.java
##########
@@ -22,7 +22,7 @@
* The default compaction strategy for user initiated compactions. This strategy will always select
* all files.
*/
-
+@SuppressWarnings("removal")
Review comment:
You don't want warnings for this class? It is no longer used with this change.
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -646,10 +690,30 @@
"After a tablet has been idle (no mutations) for this time period it may have its "
+ "in-memory map flushed to disk in a minor compaction. There is no guarantee an idle "
+ "tablet will be compacted."),
- TABLE_MINC_MAX_MERGE_FILE_SIZE("table.compaction.minor.merge.file.size.max", "0",
- PropertyType.BYTES,
- "The max RFile size used for a merging minor compaction. The default"
- + " value of 0 disables a max file size."),
+ TABLE_COMPACTION_DISPATCHER("table.compaction.dispatcher",
+ SimpleCompactionDispatcher.class.getName(), PropertyType.CLASSNAME,
+ "A configurable dispatcher that decides what comaction service a table should use."),
Review comment:
```suggestion
"A configurable dispatcher that decides what compaction service a table should use."),
```
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/UserCompactionUtils.java
##########
@@ -0,0 +1,291 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.clientImpl;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.DataInput;
+import java.io.DataInputStream;
+import java.io.DataOutput;
+import java.io.DataOutputStream;
+import java.io.IOException;
+import java.io.UncheckedIOException;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Map.Entry;
+
+import org.apache.accumulo.core.client.IteratorSetting;
+import org.apache.accumulo.core.client.admin.CompactionConfig;
+import org.apache.accumulo.core.client.admin.PluginConfig;
+import org.apache.hadoop.io.Text;
+
+import com.google.common.base.Preconditions;
+
+public class UserCompactionUtils {
+
+ private static final int MAGIC = 0x02040810;
+ private static final int SELECTOR_MAGIC = 0xae9270bf;
+ private static final int CONFIGURER_MAGIC = 0xf93e570a;
+
+ public static final PluginConfig DEFAULT_CCC = new PluginConfig("", Map.of());
+ public static final PluginConfig DEFAULT_CSC = new PluginConfig("", Map.of());
Review comment:
I think these names weren't updated after previous changes. (see other suggestion)
##########
File path: core/src/main/java/org/apache/accumulo/core/spi/compaction/CompactionServices.java
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.spi.compaction;
+
+import java.util.Set;
+
+/**
+ * @since 2.1.0
+ */
+public interface CompactionServices {
Review comment:
Should have brief description.
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/ActiveCompaction.java
##########
@@ -35,9 +35,9 @@
*/
MINOR,
/**
- * compaction to flush a tablets memory and merge it with the tablets smallest file. This type
- * compaction is done when a tablet has too many files
+ * Accumulo no longer does merging minor compactions.
Review comment:
Is all the work for removing merging minor compactions done here?
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/CompactionConfig.java
##########
@@ -21,25 +21,37 @@
import static java.util.Objects.requireNonNull;
import static org.apache.accumulo.core.clientImpl.CompactionStrategyConfigUtil.DEFAULT_STRATEGY;
-import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
+import java.util.Map;
+import java.util.function.BooleanSupplier;
import org.apache.accumulo.core.client.IteratorSetting;
+import org.apache.accumulo.core.client.admin.compaction.CompactionConfigurer;
+import org.apache.accumulo.core.client.admin.compaction.CompactionSelector;
+import org.apache.accumulo.core.clientImpl.CompactionStrategyConfigUtil;
+import org.apache.accumulo.core.clientImpl.UserCompactionUtils;
import org.apache.hadoop.io.Text;
+import com.google.common.base.Preconditions;
+
/**
* This class exist to pass parameters to {@link TableOperations#compact(String, CompactionConfig)}
*
* @since 1.7.0
*/
public class CompactionConfig {
+
private Text start = null;
private Text end = null;
private boolean flush = true;
private boolean wait = true;
private List<IteratorSetting> iterators = Collections.emptyList();
+ @SuppressWarnings("removal")
private CompactionStrategyConfig compactionStrategy = DEFAULT_STRATEGY;
+ private Map<String,String> hints = Map.of();
+ private PluginConfig selectorConfig = UserCompactionUtils.DEFAULT_CSC;
+ private PluginConfig configurerConfig = UserCompactionUtils.DEFAULT_CCC;
Review comment:
```suggestion
private PluginConfig configurerConfig = UserCompactionUtils.DEFAULT_CONFIGURER;
```
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/compaction/TooManyDeletesSelector.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.accumulo.core.client.admin.compaction;
+
+import static org.apache.accumulo.core.client.summary.summarizers.DeletesSummarizer.DELETES_STAT;
+import static org.apache.accumulo.core.client.summary.summarizers.DeletesSummarizer.TOTAL_STAT;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.function.Predicate;
+
+import org.apache.accumulo.core.client.rfile.RFile.WriterOptions;
+import org.apache.accumulo.core.client.summary.SummarizerConfiguration;
+import org.apache.accumulo.core.client.summary.Summary;
+import org.apache.accumulo.core.client.summary.summarizers.DeletesSummarizer;
+
+/**
+ * This compaction selector works in concert with the {@link DeletesSummarizer}. Using the
+ * statistics from DeleteSummarizer this strategy will compact all files in a table when the number
+ * of deletes/non-deletes exceeds a threshold.
+ *
+ * <p>
+ * This strategy has two options. First the {@value #THRESHOLD_OPT} option allows setting the point
+ * at which a compaction will be triggered. This options defaults to {@value #THRESHOLD_OPT_DEFAULT}
+ * and must be in the range (0.0, 1.0]. The second option is {@value #PROCEED_ZERO_NO_SUMMARY_OPT}
+ * which determines if the strategy should proceed when a bulk imported file has no summary
+ * information.
+ *
+ * <p>
+ * If the delete summarizer was configured on a table that already had files, then those files will
+ * have not summary information. This strategy can still proceed in this situation. It will fall
+ * back to using Accumulo's estimated entries per file in this case. For the files without summary
+ * information the estimated number of deletes will be zero. This fall back method will
+ * underestimate deletes which will not lead to false positives, except for the case of bulk
+ * imported files. Accumulo estimates that bulk imported files have zero entires. The second option
Review comment:
```suggestion
* imported files. Accumulo estimates that bulk imported files have zero entries. The second option
```
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -410,6 +408,53 @@
"The number of threads for the metadata table scan executor."),
TSERV_MIGRATE_MAXCONCURRENT("tserver.migrations.concurrent.max", "1", PropertyType.COUNT,
"The maximum number of concurrent tablet migrations for a tablet server"),
+ TSERV_MAJC_DELAY("tserver.compaction.major.delay", "30s", PropertyType.TIMEDURATION,
+ "Time a tablet server will sleep between checking which tablets need compaction."),
+ TSERV_COMPACTION_SERVICE_PREFIX("tserver.compaction.major.service.", null, PropertyType.PREFIX,
+ "Prefix for compaction services."),
+ 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_MAX_OPEN(
+ "tserver.compaction.major.service.root.planner.opts.maxOpen", "30", PropertyType.COUNT,
+ "The maximum number of files a compaction will open"),
+ TSERV_COMPACTION_SERVICE_ROOT_EXECUTORS(
+ "tserver.compaction.major.service.root.planner.opts.executors",
+ "[{'name':'small','maxSize':'32M','numThreads':1},"
+ + "{'name':'huge','numThreads':1}]".replaceAll("'", "\""),
+ PropertyType.STRING,
+ "See {% jlink -f org.apache.accumulo.core.spi.compaction.DefaultCompactionPlanner %} "),
+ TSERV_COMPACTION_SERVICE_META_PLANNER("tserver.compaction.major.service.meta.planner",
+ DefaultCompactionPlanner.class.getName(), PropertyType.CLASSNAME,
+ "Compaction planner for metadatat table"),
Review comment:
```suggestion
"Compaction planner for metadata table"),
```
##########
File path: core/src/main/java/org/apache/accumulo/core/spi/compaction/CompactionsDirectiveImpl.java
##########
@@ -0,0 +1,85 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.spi.compaction;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.spi.compaction.CompactionDirectives.Builder;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * This class intentionally package private. This implementation is odd because it supports zero
+ * object allocations for {@code CompactionDirectives.builder().build()}.
+ */
+class CompactionsDirectiveImpl implements Builder, CompactionDirectives {
Review comment:
Should this class be in this package?
##########
File path: core/src/main/java/org/apache/accumulo/core/singletons/SingletonManager.java
##########
@@ -202,5 +202,4 @@ private static void transition() {
enabled = true;
}
}
-
}
Review comment:
Looks like no changes to this file
##########
File path: core/src/main/java/org/apache/accumulo/core/conf/Property.java
##########
@@ -646,10 +690,30 @@
"After a tablet has been idle (no mutations) for this time period it may have its "
+ "in-memory map flushed to disk in a minor compaction. There is no guarantee an idle "
+ "tablet will be compacted."),
- TABLE_MINC_MAX_MERGE_FILE_SIZE("table.compaction.minor.merge.file.size.max", "0",
- PropertyType.BYTES,
- "The max RFile size used for a merging minor compaction. The default"
- + " value of 0 disables a max file size."),
+ TABLE_COMPACTION_DISPATCHER("table.compaction.dispatcher",
+ SimpleCompactionDispatcher.class.getName(), PropertyType.CLASSNAME,
+ "A configurable dispatcher that decides what comaction service a table should use."),
+ TABLE_COMPACTION_DISPATCHER_OPTS("table.compaction.dispatcher.opts.", null, PropertyType.PREFIX,
+ "Options for the table compaction dispatcher"),
+ TABLE_COMPACTION_SELECTOR("table.compaction.selector", "", PropertyType.CLASSNAME,
+ "A configurable selector for a table that can periodically select file for mandatory "
+ + "compaction, even if the files do not meet the compaction ratio."),
+ TABLE_COMPACTION_SELECTOR_OPTS("table.compaction.selector.opts.", null, PropertyType.PREFIX,
+ "Options for the table compaction dispatcher"),
+ TABLE_COMPACTION_CONFIGURER("table.compaction.configurer", "", PropertyType.CLASSNAME,
+ "A plugin that can dynamically configure compaction output files based on input files."),
+ TABLE_COMPACTION_CONFIGURER_OPTS("table.compaction.configurer.opts.", null, PropertyType.PREFIX,
+ "Options for the table compaction configuror"),
Review comment:
```suggestion
"Options for the table compaction configurer"),
```
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/compaction/CompactableFile.java
##########
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin.compaction;
+
+import java.net.URI;
+
+import org.apache.accumulo.core.metadata.CompactableFileImpl;
+
+/**
+ * @since 2.1.0
+ */
Review comment:
API javadoc needs description.
##########
File path: core/src/main/java/org/apache/accumulo/core/spi/compaction/CompactionDirectives.java
##########
@@ -0,0 +1,48 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.spi.compaction;
+
+/**
+ * The directions of a {@link CompactionDispatcher}
Review comment:
Not sure what this means, javadoc could be improved.
##########
File path: core/src/main/java/org/apache/accumulo/core/spi/compaction/CompactionKind.java
##########
@@ -0,0 +1,45 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.spi.compaction;
+
+import org.apache.accumulo.core.client.admin.compaction.CompactionSelector;
+
+/**
+ * @since 2.1.0
+ * @see org.apache.accumulo.core.spi.compaction
+ */
Review comment:
Javadoc description needed for public facing SPI.
##########
File path: core/src/main/java/org/apache/accumulo/core/spi/compaction/SimpleCompactionDispatcher.java
##########
@@ -0,0 +1,120 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.spi.compaction;
+
+import java.util.EnumMap;
+import java.util.HashMap;
+import java.util.Map;
+
+import org.apache.accumulo.core.client.admin.CompactionConfig;
+
+/**
+ * Dispatcher that supports simple configuration for making tables use compaction services. By
+ * default it dispatches to a compction service named default.
+ *
+ * <p>
+ * The following schema is supported for configration options.
Review comment:
```suggestion
* default it dispatches to a compaction service named default.
*
* <p>
* The following schema is supported for configuration options.
```
##########
File path: core/src/main/java/org/apache/accumulo/core/spi/compaction/CompactionPlanner.java
##########
@@ -0,0 +1,191 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.spi.compaction;
+
+import java.util.Collection;
+import java.util.Map;
+
+import org.apache.accumulo.core.client.admin.CompactionConfig;
+import org.apache.accumulo.core.client.admin.compaction.CompactableFile;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.spi.common.ServiceEnvironment;
+
+/**
+ * Plans compaction work for a compaction service.
+ *
+ * @since 2.1.0
+ * @see org.apache.accumulo.core.spi.compaction
+ */
+public interface CompactionPlanner {
+
+ /*
Review comment:
```suggestion
/**
```
##########
File path: core/src/main/java/org/apache/accumulo/core/spi/compaction/package-info.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+/**
+ * This package provides a place for plugin interfaces related to executing compactions. The diagram
+ * below shows the functional components in Accumulo related to compactions. Not all of these
+ * components are pluggable, but understanding how everything fits together is important for writing
+ * a plugin.
+ *
+ * <p>
+ * <img src="doc-files/compaction-spi-design.png" alt="Compaction design diagram">
+ *
+ * <p>
+ * The following is a desciption of each functional component.
+ *
+ * <ul>
+ * <li><b>Compaction Manager</b> A non pluggable component within the tablet server that brings all
+ * other components together. The manager will route compactables to compaction services. For each
+ * kind of compaction, an individual compactible will be routed to a single compaction service. For
Review comment:
```suggestion
* The following is a description of each functional component.
*
* <ul>
* <li><b>Compaction Manager</b> A non pluggable component within the tablet server that brings all
* other components together. The manager will route compactables to compaction services. For each
* kind of compaction, an individual compactable will be routed to a single compaction service. For
```
##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java
##########
@@ -239,12 +238,7 @@ public static StoredTabletFile updateTabletDataFile(ServerContext context, KeyEx
}
tablet.putFlushId(flushId);
- if (mergeFile != null) {
- tablet.deleteFile(mergeFile);
- }
-
unusedWalLogs.forEach(tablet::deleteWal);
- filesInUseByScans.forEach(tablet::putScan);
Review comment:
This related to removal for merging minor compactions?
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r440381804
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/compaction/TooManyDeletesSelector.java
##########
@@ -0,0 +1,157 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.accumulo.core.client.admin.compaction;
+
+import static org.apache.accumulo.core.client.summary.summarizers.DeletesSummarizer.DELETES_STAT;
+import static org.apache.accumulo.core.client.summary.summarizers.DeletesSummarizer.TOTAL_STAT;
+
+import java.util.Collection;
+import java.util.List;
+import java.util.function.Predicate;
+
+import org.apache.accumulo.core.client.rfile.RFile.WriterOptions;
+import org.apache.accumulo.core.client.summary.SummarizerConfiguration;
+import org.apache.accumulo.core.client.summary.Summary;
+import org.apache.accumulo.core.client.summary.summarizers.DeletesSummarizer;
+
+/**
+ * This compaction selector works in concert with the {@link DeletesSummarizer}. Using the
+ * statistics from DeleteSummarizer this strategy will compact all files in a table when the number
+ * of deletes/non-deletes exceeds a threshold.
+ *
+ * <p>
+ * This strategy has two options. First the {@value #THRESHOLD_OPT} option allows setting the point
+ * at which a compaction will be triggered. This options defaults to {@value #THRESHOLD_OPT_DEFAULT}
+ * and must be in the range (0.0, 1.0]. The second option is {@value #PROCEED_ZERO_NO_SUMMARY_OPT}
+ * which determines if the strategy should proceed when a bulk imported file has no summary
+ * information.
+ *
+ * <p>
+ * If the delete summarizer was configured on a table that already had files, then those files will
+ * have not summary information. This strategy can still proceed in this situation. It will fall
+ * back to using Accumulo's estimated entries per file in this case. For the files without summary
+ * information the estimated number of deletes will be zero. This fall back method will
+ * underestimate deletes which will not lead to false positives, except for the case of bulk
+ * imported files. Accumulo estimates that bulk imported files have zero entires. The second option
+ * {@value #PROCEED_ZERO_NO_SUMMARY_OPT} determines if this strategy should proceed when it sees
+ * bulk imported files that do not have summary data. This option defaults to
+ * {@value #PROCEED_ZERO_NO_SUMMARY_OPT_DEFAULT}.
+ *
+ * <p>
+ * Bulk files can be generated with summary information by calling
+ * {@code AccumuloFileOutputFormat#setSummarizers(JobConf, SummarizerConfiguration...)} or
+ * {@link WriterOptions#withSummarizers(SummarizerConfiguration...)}
+ *
+ * <p>
+ * When using this feature, its important to ensure summary cache is on and the summaries fit in the
+ * cache.
+ *
+ * @since 2.1.0
+ */
+public class TooManyDeletesSelector implements CompactionSelector {
+
+ private double threshold;
+
+ private boolean proceed_bns;
+
+ /**
+ * This option should be a floating point number between 1 and 0.
+ */
+ public static final String THRESHOLD_OPT = "threshold";
+
+ /**
+ * The default threshold.
+ */
+ public static final String THRESHOLD_OPT_DEFAULT = ".25";
+
+ public static final String PROCEED_ZERO_NO_SUMMARY_OPT = "proceed_zero_no_summary";
+
+ public static final String PROCEED_ZERO_NO_SUMMARY_OPT_DEFAULT = "false";
+
+ @Override
+ public void init(InitParamaters iparams) {
+ var options = iparams.getOptions();
+ this.threshold = Double.parseDouble(options.getOrDefault(THRESHOLD_OPT, THRESHOLD_OPT_DEFAULT));
+ if (threshold <= 0.0 || threshold > 1.0) {
+ throw new IllegalArgumentException(
+ "Threshold must be in range (0.0, 1.0], saw : " + threshold);
+ }
+
+ this.proceed_bns = Boolean.parseBoolean(
+ options.getOrDefault(PROCEED_ZERO_NO_SUMMARY_OPT, PROCEED_ZERO_NO_SUMMARY_OPT_DEFAULT));
+ }
+
+ @Override
+ public Selection select(SelectionParameters sparams) {
+
+ var tableConf = sparams.getEnvironment().getConfiguration(sparams.getTableId());
+
+ // TODO ISSUE could add a method to get props with prefix. That could be used to efficiently get
Review comment:
Open issue for TODO
##########
File path: core/src/main/java/org/apache/accumulo/core/client/admin/compaction/CompactionSelector.java
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin.compaction;
+
+import java.util.Collection;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.function.Predicate;
+
+import org.apache.accumulo.core.client.PluginEnvironment;
+import org.apache.accumulo.core.client.sample.SamplerConfiguration;
+import org.apache.accumulo.core.client.summary.SummarizerConfiguration;
+import org.apache.accumulo.core.client.summary.Summary;
+import org.apache.accumulo.core.data.Key;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.data.Value;
+import org.apache.accumulo.core.iterators.SortedKeyValueIterator;
+
+/**
+ * This class select which files a user compaction will compact. It can also be configured per table
Review comment:
Typo
```suggestion
* This class selects which files a user compaction will compact. It can also be configured per table
```
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionService.java
##########
@@ -0,0 +1,269 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.tserver.compactions;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.function.Consumer;
+
+import org.apache.accumulo.core.client.admin.compaction.CompactableFile;
+import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
+import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.dataImpl.KeyExtent;
+import org.apache.accumulo.core.spi.common.ServiceEnvironment;
+import org.apache.accumulo.core.spi.compaction.CompactionExecutorId;
+import org.apache.accumulo.core.spi.compaction.CompactionJob;
+import org.apache.accumulo.core.spi.compaction.CompactionKind;
+import org.apache.accumulo.core.spi.compaction.CompactionPlan;
+import org.apache.accumulo.core.spi.compaction.CompactionPlanner;
+import org.apache.accumulo.core.spi.compaction.CompactionPlanner.PlanningParameters;
+import org.apache.accumulo.core.spi.compaction.CompactionServiceId;
+import org.apache.accumulo.core.spi.compaction.ExecutorManager;
+import org.apache.accumulo.core.util.compaction.CompactionPlanImpl;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.accumulo.server.ServiceEnvironmentImpl;
+import org.apache.accumulo.tserver.TabletServerResourceManager;
+import org.apache.accumulo.tserver.compactions.SubmittedJob.Status;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.common.base.Preconditions;
+
+public class CompactionService {
+ // TODO ISSUE move rate limiters to the compaction service level.
+ private final CompactionPlanner planner;
+ private final Map<CompactionExecutorId,CompactionExecutor> executors;
+ private final CompactionServiceId myId;
+ private Map<KeyExtent,Collection<SubmittedJob>> submittedJobs = new ConcurrentHashMap<>();
+ private ServerContext serverCtx;
+
+ private static final Logger log = LoggerFactory.getLogger(CompactionService.class);
+
+ // TODO ISSUE change thread pool sizes if compaction service config changes
+ public CompactionService(String serviceName, String plannerClass,
+ Map<String,String> serviceOptions, ServerContext sctx, TabletServerResourceManager tsrm) {
+
+ this.myId = CompactionServiceId.of(serviceName);
+ this.serverCtx = sctx;
+
+ try {
+ planner =
+ ConfigurationTypeHelper.getClassInstance(null, plannerClass, CompactionPlanner.class);
+ } catch (IOException | ReflectiveOperationException e) {
+ throw new RuntimeException(e);
+ }
+
+ Map<CompactionExecutorId,CompactionExecutor> tmpExecutors = new HashMap<>();
+
+ planner.init(new CompactionPlanner.InitParameters() {
+
+ @Override
+ public ServiceEnvironment getServiceEnvironment() {
+ return new ServiceEnvironmentImpl(sctx);
+ }
+
+ @Override
+ public Map<String,String> getOptions() {
+ return serviceOptions;
+ }
+
+ @Override
+ public ExecutorManager getExecutorManager() {
+ return new ExecutorManager() {
+ @Override
+ public CompactionExecutorId createExecutor(String executorName, int threads) {
+ var ceid = CompactionExecutorId.of(serviceName + "." + executorName);
+ Preconditions.checkState(!tmpExecutors.containsKey(ceid));
+ tmpExecutors.put(ceid, new CompactionExecutor(ceid, threads, tsrm));
+ return ceid;
+ }
+ };
+ }
+
+ @Override
+ public String getFullyQualifiedOption(String key) {
+ return Property.TSERV_COMPACTION_SERVICE_PREFIX.getKey() + serviceName + ".opts." + key;
+ }
+ });
+
+ this.executors = Map.copyOf(tmpExecutors);
+
+ log.debug("Created new compaction service id:{} executors:{}", myId, executors.keySet());
+ }
+
+ private boolean reconcile(Set<CompactionJob> jobs, Collection<SubmittedJob> submitted) {
+ for (SubmittedJob submittedJob : submitted) {
+ // only read status once to avoid race conditions since multiple compares are done
+ var status = submittedJob.getStatus();
+ if (status == Status.QUEUED) {
+ if (!jobs.remove(submittedJob.getJob())) {
+ if (!submittedJob.cancel(Status.QUEUED)) {
+ return false;
+ }
+ }
+ } else if (status == Status.RUNNING) {
+ for (CompactionJob job : jobs) {
+ if (!Collections.disjoint(submittedJob.getJob().getFiles(), job.getFiles())) {
+ return false;
+ }
+ }
+ }
+ }
+
+ return true;
+ }
+
+ public void compact(CompactionKind kind, Compactable compactable,
+ Consumer<Compactable> completionCallback) {
+ // TODO ISSUE this could take a while... could run this in a thread pool
Review comment:
Open issue for TODO
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionService.java
##########
@@ -0,0 +1,269 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.tserver.compactions;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.function.Consumer;
+
+import org.apache.accumulo.core.client.admin.compaction.CompactableFile;
+import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
+import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.dataImpl.KeyExtent;
+import org.apache.accumulo.core.spi.common.ServiceEnvironment;
+import org.apache.accumulo.core.spi.compaction.CompactionExecutorId;
+import org.apache.accumulo.core.spi.compaction.CompactionJob;
+import org.apache.accumulo.core.spi.compaction.CompactionKind;
+import org.apache.accumulo.core.spi.compaction.CompactionPlan;
+import org.apache.accumulo.core.spi.compaction.CompactionPlanner;
+import org.apache.accumulo.core.spi.compaction.CompactionPlanner.PlanningParameters;
+import org.apache.accumulo.core.spi.compaction.CompactionServiceId;
+import org.apache.accumulo.core.spi.compaction.ExecutorManager;
+import org.apache.accumulo.core.util.compaction.CompactionPlanImpl;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.accumulo.server.ServiceEnvironmentImpl;
+import org.apache.accumulo.tserver.TabletServerResourceManager;
+import org.apache.accumulo.tserver.compactions.SubmittedJob.Status;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.common.base.Preconditions;
+
+public class CompactionService {
+ // TODO ISSUE move rate limiters to the compaction service level.
Review comment:
Open issue for TODO
##########
File path: server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java
##########
@@ -467,14 +465,11 @@ public String invalidMessage(String argument) {
}
case TABLE_COMPACT: {
TableOperation tableOp = TableOperation.COMPACT;
- validateArgumentCount(arguments, tableOp, 5);
+ // TODO ISSUE could have compatability mode for the old number of args
Review comment:
Open Issue for TODO
##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/compactions/CompactionService.java
##########
@@ -0,0 +1,269 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.tserver.compactions;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.function.Consumer;
+
+import org.apache.accumulo.core.client.admin.compaction.CompactableFile;
+import org.apache.accumulo.core.conf.ConfigurationTypeHelper;
+import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.dataImpl.KeyExtent;
+import org.apache.accumulo.core.spi.common.ServiceEnvironment;
+import org.apache.accumulo.core.spi.compaction.CompactionExecutorId;
+import org.apache.accumulo.core.spi.compaction.CompactionJob;
+import org.apache.accumulo.core.spi.compaction.CompactionKind;
+import org.apache.accumulo.core.spi.compaction.CompactionPlan;
+import org.apache.accumulo.core.spi.compaction.CompactionPlanner;
+import org.apache.accumulo.core.spi.compaction.CompactionPlanner.PlanningParameters;
+import org.apache.accumulo.core.spi.compaction.CompactionServiceId;
+import org.apache.accumulo.core.spi.compaction.ExecutorManager;
+import org.apache.accumulo.core.util.compaction.CompactionPlanImpl;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.accumulo.server.ServiceEnvironmentImpl;
+import org.apache.accumulo.tserver.TabletServerResourceManager;
+import org.apache.accumulo.tserver.compactions.SubmittedJob.Status;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.common.base.Preconditions;
+
+public class CompactionService {
+ // TODO ISSUE move rate limiters to the compaction service level.
+ private final CompactionPlanner planner;
+ private final Map<CompactionExecutorId,CompactionExecutor> executors;
+ private final CompactionServiceId myId;
+ private Map<KeyExtent,Collection<SubmittedJob>> submittedJobs = new ConcurrentHashMap<>();
+ private ServerContext serverCtx;
+
+ private static final Logger log = LoggerFactory.getLogger(CompactionService.class);
+
+ // TODO ISSUE change thread pool sizes if compaction service config changes
Review comment:
Open issue for TODO
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r422262167
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/CompactionStrategyConfigUtil.java
##########
@@ -18,23 +18,17 @@
*/
package org.apache.accumulo.core.clientImpl;
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
import java.io.DataInput;
-import java.io.DataInputStream;
import java.io.DataOutput;
-import java.io.DataOutputStream;
-import java.io.IOException;
-import java.util.HashMap;
import java.util.Map;
-import java.util.Map.Entry;
+import org.apache.accumulo.core.client.admin.CompactionConfig;
import org.apache.accumulo.core.client.admin.CompactionStrategyConfig;
+@SuppressWarnings("removal")
Review comment:
This entire class is utility code a deprecated class.
----------------------------------------------------------------
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 #1605: Fixes #564 adds support multiple compaction executors
Posted by GitBox <gi...@apache.org>.
keith-turner commented on a change in pull request #1605:
URL: https://github.com/apache/accumulo/pull/1605#discussion_r433507764
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/CompactionStrategyConfigUtil.java
##########
@@ -18,23 +18,17 @@
*/
package org.apache.accumulo.core.clientImpl;
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
import java.io.DataInput;
-import java.io.DataInputStream;
import java.io.DataOutput;
-import java.io.DataOutputStream;
-import java.io.IOException;
-import java.util.HashMap;
import java.util.Map;
-import java.util.Map.Entry;
+import org.apache.accumulo.core.client.admin.CompactionConfig;
import org.apache.accumulo.core.client.admin.CompactionStrategyConfig;
+@SuppressWarnings("removal")
Review comment:
@ctubbsii does my answer address your concern?
----------------------------------------------------------------
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