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