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:
[email protected]
