Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r486974595
##########
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:
The replacement code is my changes in #1616. That is what spawned most
of these things. Though maybe this is still considered necessary but if
lastLocation is properly set through those other changes then it doesn't have
to be set here. I believe after #1616 this function wasn't used anymore but it
has been awhile, I can go back and confirm that aspect.
##########
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:
Same, as above.
##########
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:
I will add that. If I recall correctly, I was running into a bug where
the portString was slightly off and I was getting the unparseable port number
exception and that is why I needed to add this. I need to confirm if that is
still necessary.
##########
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 will add this back. Intellij listed DistrubedStoreException as not
being used after my changes which is why I removed it in the first place.
##########
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:
I am not too sure about the performance implications. I remember this
part being one of the ones I struggled with trying to figure out how to apply
my other changes too. Part of the original goal of making these changes to the
state stores was to pass in a single assignment instead of a collection, along
with applying ample to it. Some of the state stores already did this before
these changes like in ZooTabletStateStore.setFutureLocation. Its possible the
easiest and best solution is to add the collection<assignment> back and use:
'''java
if (assignments.size() != 1)
throw new IllegalArgumentException("There is only one root tablet");
Assignment assignment = assignments.iterator().next();
if (assignment.tablet.compareTo(RootTable.EXTENT) != 0)
throw new IllegalArgumentException("You can only store the root tablet
location");
'''
at the beginning of the various state store functions.
##########
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:
I am not too sure about the performance implications. I remember this
part being one of the ones I struggled with trying to figure out how to apply
my other changes too. Part of the original goal of making these changes to the
state stores was to pass in a single assignment instead of a collection, along
with applying ample to it. Some of the state stores already did this before
these changes like in ZooTabletStateStore.setFutureLocation. Its possible the
easiest and best solution is to add the collection<assignment> back and use:
``` java
if (assignments.size() != 1)
throw new IllegalArgumentException("There is only one root tablet");
Assignment assignment = assignments.iterator().next();
if (assignment.tablet.compareTo(RootTable.EXTENT) != 0)
throw new IllegalArgumentException("You can only store the root tablet
`location");
```
at the beginning of the various state store functions.
##########
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:
Yeah, that appears to have been a oversight on my part. I will use the
tabletMetadate to get the last location.
##########
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 added the try/catch back to these functions in MetaDataStateStore, do
you think I should do the same to the other state stores even though they
didn't have them before?
##########
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 will add this back. Intellij listed DistributedStoreException as not
being used after my changes which is why I removed it in the first place.
##########
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:
I will add that. This is to ensure it actually grabs the port number,
due to some change I made, `MasterRepairsDualAssignmentIT`fails because the
portString contains more than just the port number. Example below:
```
38277[10005df00740005]
```
I'll take a look at the cause but this if statement does solve it
temporarily.
##########
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:
That makes sense. I was told that was the case(only one assignment at a
time) for all the stores even though we implemented it to be able to handle
multiple assignments. Just to be clear, I should revert back to using
collection<assignments> for these functions?
##########
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:
I will add it this Monday.
----------------------------------------------------------------
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]
