Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
From: mjwall <git@git.apache.org>
To: dev@accumulo.apache.org
Reply-To: dev@accumulo.apache.org
References: <git-pr-107-accumulo@git.apache.org>
In-Reply-To: <git-pr-107-accumulo@git.apache.org>
Subject: [GitHub] accumulo pull request #107: ACCUMULO-4157 Bug fix for removing WALs to quick...
Content-Type: text/plain
Message-Id: <20160603131714.A45FCE083B@git1-us-west.apache.org>
Date: Fri,  3 Jun 2016 13:17:14 +0000 (UTC)
archived-at: Fri, 03 Jun 2016 13:17:17 -0000

Github user mjwall commented on a diff in the pull request:

    https://github.com/apache/accumulo/pull/107#discussion_r65704867
  
    --- Diff: server/gc/src/test/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogsTest.java ---
    @@ -234,4 +257,288 @@ public void testIsUUID() {
         assertFalse(GarbageCollectWriteAheadLogs.isUUID("0" + UUID.randomUUID().toString()));
         assertFalse(GarbageCollectWriteAheadLogs.isUUID(null));
       }
    +
    +  @Test
    +  public void testTimeToDeleteTrue() throws InterruptedException {
    +    gcwal.clearFirstSeenDead();
    +    HostAndPort address = HostAndPort.fromString("tserver1:9998");
    +    long wait = AccumuloConfiguration.getTimeInMillis("1s");
    +    gcwal.timeToDelete(address, wait); // to store first seen dead
    +    sleep(wait * 2);
    +    assertTrue(gcwal.timeToDelete(address, wait));
    +  }
    +
    +  @Test
    +  public void testTimeToDeleteFalse() {
    +    HostAndPort address = HostAndPort.fromString("tserver1:9998");
    +    long wait = AccumuloConfiguration.getTimeInMillis("1h");
    +    long t1, t2;
    +    boolean ttd;
    +    do {
    +      t1 = System.nanoTime();
    +      gcwal.clearFirstSeenDead();
    +      gcwal.timeToDelete(address, wait);
    +      ttd = gcwal.timeToDelete(address, wait);
    +      t2 = System.nanoTime();
    +    } while (t2 - t1 > (wait / 2) * 1000000); // as long as it took less than half of the configured wait
    +
    +    assertFalse(gcwal.timeToDelete(address, wait));
    +  }
    +
    +  @Test
    +  public void testTimeToDeleteWithNullAddress() {
    +    assertFalse(gcwal.timeToDelete(null, 123l));
    +  }
    +
    +  /**
    +   * Wrapper class with some helper methods
    +   * <p>
    +   * Just a wrapper around a LinkedHashMap that store method name and argument information. Also includes some convenience methods to make usage cleaner.
    +   */
    +  class MethodCalls {
    +
    +    private LinkedHashMap<String,List<Object>> mapWrapper;
    +
    +    public MethodCalls() {
    +      mapWrapper = new LinkedHashMap<String,List<Object>>();
    +    }
    +
    +    public void put(String methodName, Object... args) {
    +      mapWrapper.put(methodName, Arrays.asList(args));
    +    }
    +
    +    public int size() {
    +      return mapWrapper.size();
    +    }
    +
    +    public boolean hasOneEntry() {
    +      return size() == 1;
    +    }
    +
    +    public Map.Entry<String,List<Object>> getFirstEntry() {
    +      return mapWrapper.entrySet().iterator().next();
    +    }
    +
    +    public String getFirstEntryMethod() {
    +      return getFirstEntry().getKey();
    +    }
    +
    +    public List<Object> getFirstEntryArgs() {
    +      return getFirstEntry().getValue();
    +    }
    +
    +    public Object getFirstEntryArg(int number) {
    +      return getFirstEntryArgs().get(number);
    +    }
    +  }
    +
    +  /**
    +   * Partial mock of the GarbageCollectWriteAheadLogs for testing the removeFile method
    +   * <p>
    +   * There is a map named methodCalls that can be used to assert parameters on methods called inside the removeFile method
    +   */
    +  class GCWALPartialMock extends GarbageCollectWriteAheadLogs {
    +
    +    private boolean holdsLockBool = false;
    +
    +    public GCWALPartialMock(Instance i, VolumeManager vm, boolean useTrash, boolean holdLock) throws IOException {
    +      super(i, vm, useTrash);
    +      this.holdsLockBool = holdLock;
    +    }
    +
    +    public MethodCalls methodCalls = new MethodCalls();
    +
    +    @Override
    +    boolean holdsLock(HostAndPort addr) {
    +      return holdsLockBool;
    +    }
    +
    +    @Override
    +    void removeWALfromDownTserver(HostAndPort address, AccumuloConfiguration conf, Entry<String,ArrayList<Path>> entry, final GCStatus status) {
    +      methodCalls.put("removeWALFromDownTserver", address, conf, entry, status);
    +    }
    +
    +    @Override
    +    void askTserverToRemoveWAL(HostAndPort address, AccumuloConfiguration conf, Entry<String,ArrayList<Path>> entry, final GCStatus status) {
    +      methodCalls.put("askTserverToRemoveWAL", address, conf, entry, status);
    +    }
    +
    +    @Override
    +    void removeOldStyleWAL(Entry<String,ArrayList<Path>> entry, final GCStatus status) {
    +      methodCalls.put("removeOldStyleWAL", entry, status);
    +    }
    +
    +    @Override
    +    void removeSortedWAL(Path swalog) {
    +      methodCalls.put("removeSortedWAL", swalog);
    +    }
    +  }
    +
    +  private GCWALPartialMock getGCWALForRemoveFileTest(GCStatus s, final boolean locked) throws IOException {
    +    return new GCWALPartialMock(new MockInstance("accumulo"), VolumeManagerImpl.get(), false, locked);
    +  }
    +
    +  private Map<String,Path> getEmptyMap() {
    +    return new HashMap<String,Path>();
    +  }
    +
    +  private Map<String,ArrayList<Path>> getServerToFileMap1(String key, Path singlePath) {
    +    Map<String,ArrayList<Path>> serverToFileMap = new HashMap<String,ArrayList<Path>>();
    +    serverToFileMap.put(key, new ArrayList<Path>(Arrays.asList(singlePath)));
    +    return serverToFileMap;
    +  }
    +
    +  @Test
    +  public void testRemoveFilesWithOldStyle() throws IOException {
    +    GCStatus status = new GCStatus();
    +    GarbageCollectWriteAheadLogs realGCWAL = getGCWALForRemoveFileTest(status, true);
    +    Path p1 = new Path("hdfs://localhost:9000/accumulo/wal/tserver1+9997/" + UUID.randomUUID().toString());
    +    Map<String,ArrayList<Path>> serverToFileMap = getServerToFileMap1("", p1);
    +
    +    realGCWAL.removeFiles(getEmptyMap(), serverToFileMap, getEmptyMap(), status);
    +
    +    MethodCalls calls = ((GCWALPartialMock) realGCWAL).methodCalls;
    +    assertEquals("Only one method should have been called", 1, calls.size());
    +    assertEquals("Method should be removeOldStyleWAL", "removeOldStyleWAL", calls.getFirstEntryMethod());
    +    Entry<String,ArrayList<Path>> firstServerToFileMap = serverToFileMap.entrySet().iterator().next();
    +    assertEquals("First param should be empty", firstServerToFileMap, calls.getFirstEntryArg(0));
    +    assertEquals("Second param should be the status", status, calls.getFirstEntryArg(1));
    +  }
    +
    +  @Test
    +  public void testRemoveFilesWithDeadTservers() throws IOException {
    +    GCStatus status = new GCStatus();
    +    GarbageCollectWriteAheadLogs realGCWAL = getGCWALForRemoveFileTest(status, false);
    +    String server = "tserver1+9997";
    +    Path p1 = new Path("hdfs://localhost:9000/accumulo/wal/" + server + "/" + UUID.randomUUID().toString());
    +    Map<String,ArrayList<Path>> serverToFileMap = getServerToFileMap1(server, p1);
    +
    +    realGCWAL.removeFiles(getEmptyMap(), serverToFileMap, getEmptyMap(), status);
    +
    +    MethodCalls calls = ((GCWALPartialMock) realGCWAL).methodCalls;
    +    assertEquals("Only one method should have been called", 1, calls.size());
    +    assertEquals("Method should be removeWALfromDownTserver", "removeWALFromDownTserver", calls.getFirstEntryMethod());
    +    assertEquals("First param should be address", HostAndPort.fromString(server.replaceAll("[+]", ":")), calls.getFirstEntryArg(0));
    +    assertTrue("Second param should be an AccumuloConfiguration", calls.getFirstEntryArg(1) instanceof AccumuloConfiguration);
    +    Entry<String,ArrayList<Path>> firstServerToFileMap = serverToFileMap.entrySet().iterator().next();
    +    assertEquals("Third param should be the entry", firstServerToFileMap, calls.getFirstEntryArg(2));
    +    assertEquals("Forth param should be the status", status, calls.getFirstEntryArg(3));
    +  }
    +
    +  @Test
    +  public void testRemoveFilesWithLiveTservers() throws IOException {
    +    GCStatus status = new GCStatus();
    +    GarbageCollectWriteAheadLogs realGCWAL = getGCWALForRemoveFileTest(status, true);
    +    String server = "tserver1+9997";
    +    Path p1 = new Path("hdfs://localhost:9000/accumulo/wal/" + server + "/" + UUID.randomUUID().toString());
    +    Map<String,ArrayList<Path>> serverToFileMap = getServerToFileMap1(server, p1);
    +
    +    realGCWAL.removeFiles(getEmptyMap(), serverToFileMap, getEmptyMap(), status);
    +
    +    MethodCalls calls = ((GCWALPartialMock) realGCWAL).methodCalls;
    +    assertEquals("Only one method should have been called", 1, calls.size());
    +    assertEquals("Method should be askTserverToRemoveWAL", "askTserverToRemoveWAL", calls.getFirstEntryMethod());
    +    assertEquals("First param should be address", HostAndPort.fromString(server.replaceAll("[+]", ":")), calls.getFirstEntryArg(0));
    +    assertTrue("Second param should be an AccumuloConfiguration", calls.getFirstEntryArg(1) instanceof AccumuloConfiguration);
    +    Entry<String,ArrayList<Path>> firstServerToFileMap = serverToFileMap.entrySet().iterator().next();
    +    assertEquals("Third param should be the entry", firstServerToFileMap, calls.getFirstEntryArg(2));
    +    assertEquals("Forth param should be the status", status, calls.getFirstEntryArg(3));
    +  }
    +
    +  @Test
    +  public void testRemoveFilesRemovesSortedWALs() throws IOException {
    +    GCStatus status = new GCStatus();
    +    GarbageCollectWriteAheadLogs realGCWAL = getGCWALForRemoveFileTest(status, true);
    +    Map<String,ArrayList<Path>> serverToFileMap = new HashMap<String,ArrayList<Path>>();
    +    Map<String,Path> sortedWALogs = new HashMap<String,Path>();
    +    Path p1 = new Path("hdfs://localhost:9000/accumulo/wal/tserver1+9997/" + UUID.randomUUID().toString());
    +    sortedWALogs.put("junk", p1); // TODO: see if this key is actually used here, maybe can be removed
    +
    +    realGCWAL.removeFiles(getEmptyMap(), serverToFileMap, sortedWALogs, status);
    +    MethodCalls calls = ((GCWALPartialMock) realGCWAL).methodCalls;
    +    assertEquals("Only one method should have been called", 1, calls.size());
    +    assertEquals("Method should be removeSortedWAL", "removeSortedWAL", calls.getFirstEntryMethod());
    +    assertEquals("First param should be the Path", p1, calls.getFirstEntryArg(0));
    +
    +  }
    +
    +  static String GCWAL_DEAD_DIR = "gcwal-collect-deadtserver";
    +  static String GCWAL_DEAD_TSERVER = "tserver1";
    +  static String GCWAL_DEAD_TSERVER_PORT = "9995";
    +  static String GCWAL_DEAD_TSERVER_COLLECT_FILE = UUID.randomUUID().toString();
    +
    +  class GCWALDeadTserverCollectMock extends GarbageCollectWriteAheadLogs {
    +
    +    public GCWALDeadTserverCollectMock(Instance i, VolumeManager vm, boolean useTrash) throws IOException {
    +      super(i, vm, useTrash);
    +    }
    +
    +    @Override
    +    boolean holdsLock(HostAndPort addr) {
    +      // tries use zookeeper
    +      return false;
    +    }
    +
    +    @Override
    +    Map<String,Path> getSortedWALogs() {
    +      return new HashMap<String,Path>();
    +    }
    +
    +    @Override
    +    int scanServers(Map<Path,String> fileToServerMap, Map<String,Path> nameToFileMap) throws Exception {
    +      String sep = File.separator;
    +      Path p = new Path(System.getProperty("java.io.tmpdir") + sep + GCWAL_DEAD_DIR + sep + GCWAL_DEAD_TSERVER + "+" + GCWAL_DEAD_TSERVER_PORT + sep
    +          + GCWAL_DEAD_TSERVER_COLLECT_FILE);
    +      fileToServerMap.put(p, GCWAL_DEAD_TSERVER + ":" + GCWAL_DEAD_TSERVER_PORT);
    +      nameToFileMap.put(GCWAL_DEAD_TSERVER_COLLECT_FILE, p);
    +      return 1;
    +    }
    +
    +    @Override
    +    int removeMetadataEntries(Map<String,Path> nameToFileMap, Map<String,Path> sortedWALogs, GCStatus status) throws IOException, KeeperException,
    +        InterruptedException {
    +      return 0;
    +    }
    +
    +    long getGCWALDeadServerWaitTime(AccumuloConfiguration conf) {
    +      // tries to use zookeeper
    +      return 1000l;
    +    }
    +  }
    +
    +  @Test
    +  public void testCollectWithDeadTserver() throws IOException, InterruptedException {
    +    Instance i = new MockInstance();
    +    File walDir = new File(System.getProperty("java.io.tmpdir") + File.separator + GCWAL_DEAD_DIR);
    --- End diff --
    
    will update, thanks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---