This is an automated email from the ASF dual-hosted git repository.
ctubbsii pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/accumulo.git
The following commit(s) were added to refs/heads/master by this push:
new 6766d92 Fix #1090 Consolidate/Simplfy ServerOpts (#1330)
6766d92 is described below
commit 6766d925ef258af88fc17cae07cbef6ce4dbec26
Author: hkeebler <49656678+hkeeb...@users.noreply.github.com>
AuthorDate: Thu Sep 19 20:39:31 2019 -0400
Fix #1090 Consolidate/Simplfy ServerOpts (#1330)
* added deprecatedoption
* Fix #1090 Deprecate command option -s in GC
* Fix #1090 Deprecate command -s in GC
removed deprecated option in favor of dropping option
* Fix #1090 Deprecate command -s in GC
includes all suggested changes
---
.../org/apache/accumulo/core/cli/ConfigOpts.java | 50 ++++++++++++++++++----
.../org/apache/accumulo/core/conf/Property.java | 13 ++++++
server/gc/pom.xml | 4 --
.../apache/accumulo/gc/SimpleGarbageCollector.java | 45 ++++++++-----------
.../gc/SimpleGarbageCollectorOptsTest.java | 38 ----------------
.../accumulo/gc/SimpleGarbageCollectorTest.java | 1 +
6 files changed, 75 insertions(+), 76 deletions(-)
diff --git a/core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java
b/core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java
index e7dccf9..ca0a234 100644
--- a/core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java
+++ b/core/src/main/java/org/apache/accumulo/core/cli/ConfigOpts.java
@@ -45,6 +45,26 @@ public class ConfigOpts extends Help {
return propsPath;
}
+ // catch all for string based dropped options, including those specific to
subclassed extensions
+ // uncomment below if needed
+ // @Parameter(names = {}, hidden=true)
+ private String legacyOpts = null;
+
+ // catch all for boolean dropped options, including those specific to
subclassed extensions
+ @Parameter(names = {"-s", "--safemode"}, hidden = true)
+ private boolean legacyOptsBoolean = false;
+
+ // holds information on dealing with dropped options
+ // option -> Message describing replacement option or property
+ private static Map<String,String> LEGACY_OPTION_MSG = new HashMap<>();
+ static {
+ // garbage collector legacy options
+ LEGACY_OPTION_MSG.put("-s", "Replaced by configuration property " +
Property.GC_SAFEMODE);
+ LEGACY_OPTION_MSG.put("--safemode",
+ "Replaced by configuration property " + Property.GC_SAFEMODE);
+
+ }
+
public static class NullSplitter implements IParameterSplitter {
@Override
public List<String> split(String value) {
@@ -78,17 +98,17 @@ public class ConfigOpts extends Help {
Map<String,String> config = new HashMap<>();
for (String prop : args) {
String[] propArgs = prop.split("=", 2);
+ String key = propArgs[0].trim();
+ String value;
if (propArgs.length == 2) {
- String key = propArgs[0].trim();
- String value = propArgs[1].trim();
- if (key.isEmpty() || value.isEmpty()) {
- throw new IllegalArgumentException("Invalid command line -o option:
" + prop);
- } else {
- config.put(key, value);
- }
- } else {
+ value = propArgs[1].trim();
+ } else { // if a boolean property then it's mere existence assumes true
+ value = Property.isValidBooleanPropertyKey(key) ? "true" : "";
+ }
+ if (key.isEmpty() || value.isEmpty()) {
throw new IllegalArgumentException("Invalid command line -o option: "
+ prop);
}
+ config.put(key, value);
}
return config;
}
@@ -96,6 +116,20 @@ public class ConfigOpts extends Help {
@Override
public void parseArgs(String programName, String[] args, Object... others) {
super.parseArgs(programName, args, others);
+ if (legacyOpts != null || legacyOptsBoolean) {
+ String errMsg = "";
+ for (String option : args) {
+ if (LEGACY_OPTION_MSG.containsKey(option)) {
+ errMsg +=
+ "Option " + option + " has been dropped - " +
LEGACY_OPTION_MSG.get(option) + "\n";
+ }
+ }
+ errMsg += "See '-o' property override option";
+ // prints error to console if ran from the command line otherwise there
is no way to know that
+ // an error occurred
+ System.err.println(errMsg);
+ throw new IllegalArgumentException(errMsg);
+ }
if (getOverrides().size() > 0) {
log.info("The following configuration was set on the command line:");
for (Map.Entry<String,String> entry : getOverrides().entrySet()) {
diff --git a/core/src/main/java/org/apache/accumulo/core/conf/Property.java
b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
index 9f401b7..7be7355 100644
--- a/core/src/main/java/org/apache/accumulo/core/conf/Property.java
+++ b/core/src/main/java/org/apache/accumulo/core/conf/Property.java
@@ -523,6 +523,8 @@ public enum Property {
"Do not use the Trash, even if it is configured."),
GC_TRACE_PERCENT("gc.trace.percent", "0.01", PropertyType.FRACTION,
"Percent of gc cycles to trace"),
+ GC_SAFEMODE("gc.safemode", "false", PropertyType.BOOLEAN,
+ "Provides listing of files to be deleted but does not delete any files"),
GC_USE_FULL_COMPACTION("gc.post.metadata.action", "flush",
PropertyType.GC_POST_ACTION,
"When the gc runs it can make a lot of changes to the metadata, on
completion, "
+ " to force the changes to be written to disk, the metadata and
root tables can be flushed"
@@ -1159,6 +1161,17 @@ public enum Property {
}
/**
+ * Checks if the given property key is a valid property and is of type
boolean.
+ *
+ * @param key
+ * property key
+ * @return true if key is valid and is of type boolean, false otherwise
+ */
+ public static boolean isValidBooleanPropertyKey(String key) {
+ return validProperties.contains(key) && getPropertyByKey(key).getType() ==
PropertyType.BOOLEAN;
+ }
+
+ /**
* Checks if the given property key is for a valid table property. A valid
table property key is
* either equal to the key of some defined table property (which each start
with
* {@link #TABLE_PREFIX}) or has a prefix matching {@link
#TABLE_CONSTRAINT_PREFIX},
diff --git a/server/gc/pom.xml b/server/gc/pom.xml
index 2915b34..e7cede0 100644
--- a/server/gc/pom.xml
+++ b/server/gc/pom.xml
@@ -28,10 +28,6 @@
<description>The garbage collecting server for Apache Accumulo to clean up
unused files.</description>
<dependencies>
<dependency>
- <groupId>com.beust</groupId>
- <artifactId>jcommander</artifactId>
- </dependency>
- <dependency>
<groupId>com.google.auto.service</groupId>
<artifactId>auto-service</artifactId>
<optional>true</optional>
diff --git
a/server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java
b/server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java
index 8855cd0..62d9f62 100644
--- a/server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java
+++ b/server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java
@@ -94,7 +94,6 @@ import org.apache.zookeeper.KeeperException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import com.beust.jcommander.Parameter;
import com.google.common.collect.Iterators;
import com.google.common.collect.Maps;
import com.google.protobuf.InvalidProtocolBufferException;
@@ -105,18 +104,6 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
// the ZK lock is acquired. The server is only for metrics, there are no
concerns about clients
// using the service before the lock is acquired.
public class SimpleGarbageCollector extends AbstractServer implements Iface {
-
- /**
- * Options for the garbage collector.
- */
- static class GCOpts extends ServerOpts {
- @Parameter(names = {"-v", "--verbose"},
- description = "extra information will get printed to stdout also")
- boolean verbose = false;
- @Parameter(names = {"-s", "--safemode"}, description = "safe mode will not
delete files")
- boolean safeMode = false;
- }
-
/**
* A fraction representing how much of the JVM's available memory should be
used for gathering
* candidates.
@@ -125,21 +112,19 @@ public class SimpleGarbageCollector extends
AbstractServer implements Iface {
private static final Logger log =
LoggerFactory.getLogger(SimpleGarbageCollector.class);
- private GCOpts opts;
private ZooLock lock;
private GCStatus status =
new GCStatus(new GcCycleStats(), new GcCycleStats(), new GcCycleStats(),
new GcCycleStats());
public static void main(String[] args) throws Exception {
- try (SimpleGarbageCollector gc = new SimpleGarbageCollector(new GCOpts(),
args)) {
+ try (SimpleGarbageCollector gc = new SimpleGarbageCollector(new
ServerOpts(), args)) {
gc.runServer();
}
}
- SimpleGarbageCollector(GCOpts opts, String[] args) {
+ SimpleGarbageCollector(ServerOpts opts, String[] args) {
super("gc", opts, args);
- this.opts = opts;
final AccumuloConfiguration conf = getConfiguration();
@@ -148,8 +133,7 @@ public class SimpleGarbageCollector extends AbstractServer
implements Iface {
log.info("start delay: {} milliseconds", getStartDelay());
log.info("time delay: {} milliseconds", gcDelay);
- log.info("safemode: {}", opts.safeMode);
- log.info("verbose: {}", opts.verbose);
+ log.info("safemode: {}", inSafeMode());
log.info("memory threshold: {} of {} bytes", CANDIDATE_MEMORY_PERCENTAGE,
Runtime.getRuntime().maxMemory());
log.info("delete threads: {}", getNumDeleteThreads());
@@ -183,6 +167,15 @@ public class SimpleGarbageCollector extends AbstractServer
implements Iface {
return getConfiguration().getCount(Property.GC_DELETE_THREADS);
}
+ /**
+ * Checks if safemode is set - files will not be deleted.
+ *
+ * @return number of delete threads
+ */
+ boolean inSafeMode() {
+ return getConfiguration().getBoolean(Property.GC_SAFEMODE);
+ }
+
private class GCEnv implements GarbageCollectionEnvironment {
private DataLevel level;
@@ -263,13 +256,13 @@ public class SimpleGarbageCollector extends
AbstractServer implements Iface {
@Override
public void delete(SortedMap<String,String> confirmedDeletes) throws
TableNotFoundException {
final VolumeManager fs = getContext().getVolumeManager();
+ var metadataLocation = level == DataLevel.ROOT
+ ? getContext().getZooKeeperRoot() + " for " + RootTable.NAME :
level.metaTable();
- if (opts.safeMode) {
- if (opts.verbose) {
- System.out.println("SAFEMODE: There are " + confirmedDeletes.size()
- + " data file candidates marked for deletion.%n"
- + " Examine the log files to identify them.%n");
- }
+ if (inSafeMode()) {
+ System.out.println("SAFEMODE: There are " + confirmedDeletes.size()
+ + " data file candidates marked for deletion in " +
metadataLocation + ".\n"
+ + " Examine the log files to identify them.\n");
log.info("SAFEMODE: Listing all data file candidates for deletion");
for (String s : confirmedDeletes.values()) {
log.info("SAFEMODE: {}", s);
@@ -648,7 +641,7 @@ public class SimpleGarbageCollector extends AbstractServer
implements Iface {
processor = new Processor<>(rpcProxy);
}
int[] port = getConfiguration().getPort(Property.GC_PORT);
- HostAndPort[] addresses =
TServerUtils.getHostAndPorts(this.opts.getAddress(), port);
+ HostAndPort[] addresses = TServerUtils.getHostAndPorts(getHostname(),
port);
long maxMessageSize =
getConfiguration().getAsBytes(Property.GENERAL_MAX_MESSAGE_SIZE);
try {
ServerAddress server = TServerUtils.startTServer(getMetricsSystem(),
getConfiguration(),
diff --git
a/server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorOptsTest.java
b/server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorOptsTest.java
deleted file mode 100644
index cec83ca..0000000
---
a/server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorOptsTest.java
+++ /dev/null
@@ -1,38 +0,0 @@
-/*
- * 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.gc;
-
-import static org.junit.Assert.assertFalse;
-
-import org.apache.accumulo.gc.SimpleGarbageCollector.GCOpts;
-import org.junit.Before;
-import org.junit.Test;
-
-public class SimpleGarbageCollectorOptsTest {
- private GCOpts opts;
-
- @Before
- public void setUp() {
- opts = new GCOpts();
- }
-
- @Test
- public void testIt() {
- assertFalse(opts.verbose);
- assertFalse(opts.safeMode);
- }
-}
diff --git
a/server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java
b/server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java
index 5c2435f..a9f32b6 100644
---
a/server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java
+++
b/server/gc/src/test/java/org/apache/accumulo/gc/SimpleGarbageCollectorTest.java
@@ -94,6 +94,7 @@ public class SimpleGarbageCollectorTest {
assertTrue(gc.isUsingTrash());
assertEquals(1000L, gc.getStartDelay());
assertEquals(2, gc.getNumDeleteThreads());
+ assertFalse(gc.inSafeMode()); // false by default
}
@Test
