[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r486558996



##########
File path: core/src/main/java/org/apache/accumulo/core/util/HostAndPort.java
##########
@@ -171,6 +171,10 @@ public static HostAndPort fromString(String hostPortString) {
       // JDK7 accepts leading plus signs. We don't want to.
       checkArgument(!portString.startsWith("+"), "Unparseable port number: %s", hostPortString);
       try {
+        if (portString.contains("[")) {
+          int endIndex = portString.indexOf("[");
+          portString = portString.substring(0, endIndex);
+        }

Review comment:
       This could use a small comment above it to explain what situation it is checking for.

##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java
##########
@@ -197,13 +181,6 @@ public static void replaceDatafiles(ServerContext context, KeyExtent extent,
     if (compactionId != null)
       tablet.putCompactionId(compactionId);
 
-    TServerInstance self = getTServerInstance(address, zooLock);
-    tablet.putLocation(self, LocationType.LAST);
-
-    // remove the old location
-    if (lastLocation != null && !lastLocation.equals(self))
-      tablet.deleteLocation(lastLocation, LocationType.LAST);
-

Review comment:
       Why is it the case that these location entries no longer need to be updated here? Where is the replacement code?

##########
File path: server/manager/src/main/java/org/apache/accumulo/master/TabletGroupWatcher.java
##########
@@ -871,7 +870,9 @@ private void flushChanges(SortedMap<TServerInstance,TabletServerStatus> currentT
 
     if (!assignments.isEmpty()) {
       Master.log.info(String.format("Assigning %d tablets", assignments.size()));
-      store.setFutureLocations(assignments);
+
+      for (Assignment assignment : assignments)
+        store.setFutureLocation(assignment);

Review comment:
       It seems like this code would be less efficient, because it creates a new batch writer for each assignment, and that the method that handles the updates in Ample should deal with updating all of the assignments in one pass. What do you know about the performance implications of this change?

##########
File path: server/base/src/main/java/org/apache/accumulo/server/util/MasterMetadataUtil.java
##########
@@ -227,14 +204,6 @@ public static StoredTabletFile updateTabletDataFile(ServerContext context, KeyEx
       tablet.putFile(path, dfv);
       tablet.putTime(time);
       newFile = path.insert();
-
-      TServerInstance self = getTServerInstance(address, zooLock);
-      tablet.putLocation(self, LocationType.LAST);
-
-      // remove the old location
-      if (lastLocation != null && !lastLocation.equals(self)) {
-        tablet.deleteLocation(lastLocation, LocationType.LAST);
-      }

Review comment:
       Why is it the case that these location entries no longer need to be updated here? Where is the replacement code?

##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/AssignmentHandler.java
##########
@@ -177,7 +177,7 @@ public void run() {
         throw new RuntimeException("Minor compaction after recovery fails for " + extent);
       }
       Assignment assignment = new Assignment(extent, server.getTabletSession());
-      TabletStateStore.setLocation(server.getContext(), assignment);
+      TabletStateStore.setLocation(server.getContext(), assignment, assignment.server);

Review comment:
       Is `assignment.server` really the previously assigned location to clear? This doesn't seem right because MetaDataStateStore.setLocation just compares this `prevLastLoc` (passed in here as `assignment.server` against `assignment.server`. It seems like it would never be different, and the last location entries would remain (and possibly keep accruing?)

##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java
##########
@@ -58,137 +54,86 @@ protected MetaDataStateStore(ClientContext context, CurrentState state, String t
     return new MetaDataTableScanner(context, TabletsSection.getRange(), state, targetTableName);
   }
 
-  @Override
-  public void setLocations(Collection<Assignment> assignments) throws DistributedStoreException {
-    BatchWriter writer = createBatchWriter();
-    try {
-      for (Assignment assignment : assignments) {
-        Mutation m = new Mutation(assignment.tablet.toMetaRow());
-        assignment.server.putLocation(m);
-        assignment.server.putLastLocation(m);
-        assignment.server.clearFutureLocation(m);
-        SuspendingTServer.clearSuspension(m);
-        writer.addMutation(m);
-      }
-    } catch (Exception ex) {
-      throw new DistributedStoreException(ex);
-    } finally {
-      try {
-        writer.close();
-      } catch (MutationsRejectedException e) {
-        throw new DistributedStoreException(e);
-      }

Review comment:
       I'm not sure these catch clauses should be removed, and that DistributedStoreException should be deleted. While the conversion to Ample does eliminate the checked exception of MutationsRejectedException, by wrapping it (and all other exceptions) with a RuntimeException, the RuntimeException was previously being handled here, and now it falls through.

##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/DatafileManager.java
##########
@@ -444,7 +442,7 @@ StoredTabletFile bringMajorCompactionOnline(Set<StoredTabletFile> oldDatafiles,
 
       tablet.computeNumEntries();
 
-      lastLocation = tablet.resetLastLocation();
+      tablet.resetLastLocation();

Review comment:
       This seems likely where the actual lastLocation information is lost.

##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/UnloadTabletHandler.java
##########
@@ -123,8 +122,6 @@ public void run() {
         TabletStateStore.suspend(server.getContext(), tls, null,
             requestTimeSkew + MILLISECONDS.convert(System.nanoTime(), NANOSECONDS));
       }
-    } catch (DistributedStoreException ex) {
-      log.warn("Unable to update storage", ex);

Review comment:
       After your changes, this catch clause has no effect (since it was removed), and the RuntimeExceptions now thrown will continue falling through this, completely unhandled.




----------------------------------------------------------------
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