accumulo-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ctubb...@apache.org
Subject [accumulo] branch master updated: Fix #1090 Consolidate/Simplfy ServerOpts (#1330)
Date Fri, 20 Sep 2019 00:39:36 GMT
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+hkeebler@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


Mime
View raw message