You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2020/07/09 11:54:51 UTC
[GitHub] [accumulo] Manno15 opened a new pull request #1653: WIP. Previously Reverted Code with Fix
Manno15 opened a new pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653
This is the rest of the reverted code from pull request #1453 and includes some slight fixes for some of the IT's that it broke. This is still a work in progress due to some inconsistencies in my testing of the IT's and I know there are currently some fixes in the works for a few of those.
Most of the code changes are aimed to use TabletMutator in the various state store setLocation functions (some of these functions were already doing so but not all) and deleting unused code. Parts of these changes coincide with my change in #1616 in when we update lastLocation.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r487160077
##########
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:
Okay, that makes sense. I tried to trace the callers to this method, to see if they instead ended up reaching the new code path in #1616, but got tired last night. I'll look at it again today. I just want to make sure that if we're not updating it here, that it's because we definitely don't need to.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r487018589
##########
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?
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-691281245
@ctubbsii Yes, that makes sense. I misunderstood the intent when I was first working on this, my apologies. Thank you for taking a deeper look into it. I would like to set up a call for Monday if you are available. We can discuss how to proceed with both these issues. Again, my apologies.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r486987280
##########
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.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-692198061
I spoke with @Manno15 on Slack, and we came to the conclusion that #1616 should be reverted (which I have done) and that this PR can be rebase'd onto main and updated to reflect only the move to Ample to perform the updates (as well as removing any unneeded code that moving to Ample would obsolete, and updating Ample, as needed, to handle differently serialized port numbers and possibly to reuse a batch writer for updating multiple locations, if needed). I don't know if it is necessary to handle the RuntimeExceptions thrown by Ample and rewrapped as DistributedStoreException, or not... especially since Ample itself is written to wrap all checked exceptions and throw them through as RuntimeExceptions. If Ample doesn't think they should be handled, then maybe these stores shouldn't handle them either. I'm leaving that for @Manno15 to investigate further and come to their own conclusion.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r487154328
##########
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:
That's fine. It just needs a comment in the code.
##########
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 if it should be added to other locations. Probably not. I just know it is a behavior change to not handle the RuntimeExceptions that are thrown. I'm okay with that behavior change if it's justified (like, because we know for sure that we actually don't want to catch them here). I'm just not sure whether it's justified.
##########
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:
Well, the reason the ZooTabletStateStore only accepted one was because it can only ever have one... because the root table that uses the ZooTabletStateStore can never have more than one tablet, and therefore, there can never be more than one assignment being updated. That's not true for the other stores, because the metadata table can split into multiple tablets, and so can user tables.
##########
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:
Okay, that makes sense. I tried to trace the callers to this method, to see if they instead ended up reaching the new code path in #1616, but got tired last night. I'll look at it again today. I just want to make sure that if we're not updating it here, that it's because we definitely don't need to.
##########
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:
Based on the code you changed, it does not appear that's the case. Specifically, where you made this update in a loop, it wasn't one at a time. I think you could probably have both the single-case version, and the collection version, so we don't have to do "Collections.singletonList" everywhere when we only have one. The Collection one could be optimized to reuse the same batchwriter somehow (not sure exactly what would need to change in the tablet mutator / Ample code to do that; the old code that didn't use Ample was obviously reusing a batch writer, but I'm not sure how to do it in an Ample way). I'm also not sure how you'd pass the previous location to clear if you're passing a collection... you might have a different previous location for each assignment, right? I'm not sure without analyzing the code in a lot more detail.
##########
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:
That's fine. It just needs a comment in the code.
##########
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 if it should be added to other locations. Probably not. I just know it is a behavior change to not handle the RuntimeExceptions that are thrown. I'm okay with that behavior change if it's justified (like, because we know for sure that we actually don't want to catch them here). I'm just not sure whether it's justified.
##########
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:
Well, the reason the ZooTabletStateStore only accepted one was because it can only ever have one... because the root table that uses the ZooTabletStateStore can never have more than one tablet, and therefore, there can never be more than one assignment being updated. That's not true for the other stores, because the metadata table can split into multiple tablets, and so can user tables.
##########
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:
Okay, that makes sense. I tried to trace the callers to this method, to see if they instead ended up reaching the new code path in #1616, but got tired last night. I'll look at it again today. I just want to make sure that if we're not updating it here, that it's because we definitely don't need to.
##########
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:
Based on the code you changed, it does not appear that's the case. Specifically, where you made this update in a loop, it wasn't one at a time. I think you could probably have both the single-case version, and the collection version, so we don't have to do "Collections.singletonList" everywhere when we only have one. The Collection one could be optimized to reuse the same batchwriter somehow (not sure exactly what would need to change in the tablet mutator / Ample code to do that; the old code that didn't use Ample was obviously reusing a batch writer, but I'm not sure how to do it in an Ample way). I'm also not sure how you'd pass the previous location to clear if you're passing a collection... you might have a different previous location for each assignment, right? I'm not sure without analyzing the code in a lot more detail.
##########
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:
That's fine. It just needs a comment in the code.
##########
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 if it should be added to other locations. Probably not. I just know it is a behavior change to not handle the RuntimeExceptions that are thrown. I'm okay with that behavior change if it's justified (like, because we know for sure that we actually don't want to catch them here). I'm just not sure whether it's justified.
##########
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:
Well, the reason the ZooTabletStateStore only accepted one was because it can only ever have one... because the root table that uses the ZooTabletStateStore can never have more than one tablet, and therefore, there can never be more than one assignment being updated. That's not true for the other stores, because the metadata table can split into multiple tablets, and so can user tables.
##########
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:
Okay, that makes sense. I tried to trace the callers to this method, to see if they instead ended up reaching the new code path in #1616, but got tired last night. I'll look at it again today. I just want to make sure that if we're not updating it here, that it's because we definitely don't need to.
##########
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:
Based on the code you changed, it does not appear that's the case. Specifically, where you made this update in a loop, it wasn't one at a time. I think you could probably have both the single-case version, and the collection version, so we don't have to do "Collections.singletonList" everywhere when we only have one. The Collection one could be optimized to reuse the same batchwriter somehow (not sure exactly what would need to change in the tablet mutator / Ample code to do that; the old code that didn't use Ample was obviously reusing a batch writer, but I'm not sure how to do it in an Ample way). I'm also not sure how you'd pass the previous location to clear if you're passing a collection... you might have a different previous location for each assignment, right? I'm not sure without analyzing the code in a lot more detail.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-693586350
All the ITs passed when I ran them today. So, I'll merge this now. Thanks @Manno15 !
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r487154328
##########
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:
That's fine. It just needs a comment in the code.
##########
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 if it should be added to other locations. Probably not. I just know it is a behavior change to not handle the RuntimeExceptions that are thrown. I'm okay with that behavior change if it's justified (like, because we know for sure that we actually don't want to catch them here). I'm just not sure whether it's justified.
##########
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:
Well, the reason the ZooTabletStateStore only accepted one was because it can only ever have one... because the root table that uses the ZooTabletStateStore can never have more than one tablet, and therefore, there can never be more than one assignment being updated. That's not true for the other stores, because the metadata table can split into multiple tablets, and so can user tables.
##########
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:
Okay, that makes sense. I tried to trace the callers to this method, to see if they instead ended up reaching the new code path in #1616, but got tired last night. I'll look at it again today. I just want to make sure that if we're not updating it here, that it's because we definitely don't need to.
##########
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:
Based on the code you changed, it does not appear that's the case. Specifically, where you made this update in a loop, it wasn't one at a time. I think you could probably have both the single-case version, and the collection version, so we don't have to do "Collections.singletonList" everywhere when we only have one. The Collection one could be optimized to reuse the same batchwriter somehow (not sure exactly what would need to change in the tablet mutator / Ample code to do that; the old code that didn't use Ample was obviously reusing a batch writer, but I'm not sure how to do it in an Ample way). I'm also not sure how you'd pass the previous location to clear if you're passing a collection... you might have a different previous location for each assignment, right? I'm not sure without analyzing the code in a lot more detail.
##########
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:
That's fine. It just needs a comment in the code.
##########
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 if it should be added to other locations. Probably not. I just know it is a behavior change to not handle the RuntimeExceptions that are thrown. I'm okay with that behavior change if it's justified (like, because we know for sure that we actually don't want to catch them here). I'm just not sure whether it's justified.
##########
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:
Well, the reason the ZooTabletStateStore only accepted one was because it can only ever have one... because the root table that uses the ZooTabletStateStore can never have more than one tablet, and therefore, there can never be more than one assignment being updated. That's not true for the other stores, because the metadata table can split into multiple tablets, and so can user tables.
##########
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:
Okay, that makes sense. I tried to trace the callers to this method, to see if they instead ended up reaching the new code path in #1616, but got tired last night. I'll look at it again today. I just want to make sure that if we're not updating it here, that it's because we definitely don't need to.
##########
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:
Based on the code you changed, it does not appear that's the case. Specifically, where you made this update in a loop, it wasn't one at a time. I think you could probably have both the single-case version, and the collection version, so we don't have to do "Collections.singletonList" everywhere when we only have one. The Collection one could be optimized to reuse the same batchwriter somehow (not sure exactly what would need to change in the tablet mutator / Ample code to do that; the old code that didn't use Ample was obviously reusing a batch writer, but I'm not sure how to do it in an Ample way). I'm also not sure how you'd pass the previous location to clear if you're passing a collection... you might have a different previous location for each assignment, right? I'm not sure without analyzing the code in a lot more detail.
##########
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:
That's fine. It just needs a comment in the code.
##########
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 if it should be added to other locations. Probably not. I just know it is a behavior change to not handle the RuntimeExceptions that are thrown. I'm okay with that behavior change if it's justified (like, because we know for sure that we actually don't want to catch them here). I'm just not sure whether it's justified.
##########
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:
Well, the reason the ZooTabletStateStore only accepted one was because it can only ever have one... because the root table that uses the ZooTabletStateStore can never have more than one tablet, and therefore, there can never be more than one assignment being updated. That's not true for the other stores, because the metadata table can split into multiple tablets, and so can user tables.
##########
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:
Okay, that makes sense. I tried to trace the callers to this method, to see if they instead ended up reaching the new code path in #1616, but got tired last night. I'll look at it again today. I just want to make sure that if we're not updating it here, that it's because we definitely don't need to.
##########
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:
Based on the code you changed, it does not appear that's the case. Specifically, where you made this update in a loop, it wasn't one at a time. I think you could probably have both the single-case version, and the collection version, so we don't have to do "Collections.singletonList" everywhere when we only have one. The Collection one could be optimized to reuse the same batchwriter somehow (not sure exactly what would need to change in the tablet mutator / Ample code to do that; the old code that didn't use Ample was obviously reusing a batch writer, but I'm not sure how to do it in an Ample way). I'm also not sure how you'd pass the previous location to clear if you're passing a collection... you might have a different previous location for each assignment, right? I'm not sure without analyzing the code in a lot more detail.
##########
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:
That's fine. It just needs a comment in the code.
##########
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 if it should be added to other locations. Probably not. I just know it is a behavior change to not handle the RuntimeExceptions that are thrown. I'm okay with that behavior change if it's justified (like, because we know for sure that we actually don't want to catch them here). I'm just not sure whether it's justified.
##########
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:
Well, the reason the ZooTabletStateStore only accepted one was because it can only ever have one... because the root table that uses the ZooTabletStateStore can never have more than one tablet, and therefore, there can never be more than one assignment being updated. That's not true for the other stores, because the metadata table can split into multiple tablets, and so can user tables.
##########
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:
Okay, that makes sense. I tried to trace the callers to this method, to see if they instead ended up reaching the new code path in #1616, but got tired last night. I'll look at it again today. I just want to make sure that if we're not updating it here, that it's because we definitely don't need to.
##########
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:
Based on the code you changed, it does not appear that's the case. Specifically, where you made this update in a loop, it wasn't one at a time. I think you could probably have both the single-case version, and the collection version, so we don't have to do "Collections.singletonList" everywhere when we only have one. The Collection one could be optimized to reuse the same batchwriter somehow (not sure exactly what would need to change in the tablet mutator / Ample code to do that; the old code that didn't use Ample was obviously reusing a batch writer, but I'm not sure how to do it in an Ample way). I'm also not sure how you'd pass the previous location to clear if you're passing a collection... you might have a different previous location for each assignment, right? I'm not sure without analyzing the code in a lot more detail.
##########
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:
That's fine. It just needs a comment in the code.
##########
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 if it should be added to other locations. Probably not. I just know it is a behavior change to not handle the RuntimeExceptions that are thrown. I'm okay with that behavior change if it's justified (like, because we know for sure that we actually don't want to catch them here). I'm just not sure whether it's justified.
##########
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:
Well, the reason the ZooTabletStateStore only accepted one was because it can only ever have one... because the root table that uses the ZooTabletStateStore can never have more than one tablet, and therefore, there can never be more than one assignment being updated. That's not true for the other stores, because the metadata table can split into multiple tablets, and so can user tables.
##########
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:
Okay, that makes sense. I tried to trace the callers to this method, to see if they instead ended up reaching the new code path in #1616, but got tired last night. I'll look at it again today. I just want to make sure that if we're not updating it here, that it's because we definitely don't need to.
##########
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:
Based on the code you changed, it does not appear that's the case. Specifically, where you made this update in a loop, it wasn't one at a time. I think you could probably have both the single-case version, and the collection version, so we don't have to do "Collections.singletonList" everywhere when we only have one. The Collection one could be optimized to reuse the same batchwriter somehow (not sure exactly what would need to change in the tablet mutator / Ample code to do that; the old code that didn't use Ample was obviously reusing a batch writer, but I'm not sure how to do it in an Ample way). I'm also not sure how you'd pass the previous location to clear if you're passing a collection... you might have a different previous location for each assignment, right? I'm not sure without analyzing the code in a lot more detail.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r487200764
##########
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:
Based on the code you changed, it does not appear that's the case. Specifically, where you made this update in a loop, it wasn't one at a time. I think you could probably have both the single-case version, and the collection version, so we don't have to do "Collections.singletonList" everywhere when we only have one. The Collection one could be optimized to reuse the same batchwriter somehow (not sure exactly what would need to change in the tablet mutator / Ample code to do that; the old code that didn't use Ample was obviously reusing a batch writer, but I'm not sure how to do it in an Ample way). I'm also not sure how you'd pass the previous location to clear if you're passing a collection... you might have a different previous location for each assignment, right? I'm not sure without analyzing the code in a lot more detail.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r486991977
##########
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.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r487158983
##########
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:
Well, the reason the ZooTabletStateStore only accepted one was because it can only ever have one... because the root table that uses the ZooTabletStateStore can never have more than one tablet, and therefore, there can never be more than one assignment being updated. That's not true for the other stores, because the metadata table can split into multiple tablets, and so can user tables.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-691252150
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-691281245
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r487183075
##########
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:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-692676955
@ctubbsii I believe I made all the necessary changes to this per our discussion yesterday. I am going to run the IT's now.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-690607073
@Manno15 Thanks for this. I confirmed all the ITs passed for these changes, and am working through a code review.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r487182568
##########
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?
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-691252150
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r489363121
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java
##########
@@ -58,57 +54,31 @@ 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 {
+ try (var tabletsMutator = ample.mutateTablets()) {
for (Assignment assignment : assignments) {
- Mutation m = new Mutation(assignment.tablet.toMetaRow());
- assignment.server.putLocation(m);
- assignment.server.clearFutureLocation(m);
- SuspendingTServer.clearSuspension(m);
- writer.addMutation(m);
+ TabletMutator tabletMutator = tabletsMutator.mutateTablet(assignment.tablet);
+ tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ tabletMutator.deleteLocation(assignment.server, LocationType.FUTURE);
+ tabletMutator.mutate();
Review comment:
I'm going to go ahead and chain the methods in the other state stores and various files that use tabletMutator.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by 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
[GitHub] [accumulo] ctubbsii commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-691274564
Okay, I was trying to track down this a bit, because it didn't make sense to me that the two methods that previously updated the `LAST` location would be doing that. I traced the code and saw that the two methods in `MasterMetadataUtil` that update `LocationType.LAST` are only ever called by `DatafileManager.bringMinorCompactionOnline` and `DatafileManager.bringMajorCompactionOnline`.
So, it seems that the original intent (prior to #1616) was to *only* update the `LAST` location if a file was successfully written locally on that server. So, `LAST` does *not* represent the last assignment, but the last server where data was written from a major or minor compaction. The intent, it seems, is that the initial assignments on startup should be based on where the data would be local, and not to preserve its previous assignment.
This insight also answers the questions I had in https://github.com/apache/accumulo/pull/1616#issuecomment-665412742 about the intent of the `LAST` field. Because, if the intent was to simply assign to the last location it was previously assigned to, then assigning it to the last `CURRENT` location that was written would be sufficient (because that is updated on assignment), and we wouldn't even need a `LAST` location at all. `LAST` does not mean "last assignment", it means "last location where data was likely written locally". This is about data locality, not (re)assignment thrashing.
So, we need to go back to the drawing board here, because I'm not sure #1616 even fixed a problem at all... it simply changed behavior to re-assign to a previously assigned location, rather than the last location written to. We need to figure out if the code works as intended *without* the changes in #1616. If it does, then we need to figure out whether we want to change the behavior or not. If we do, then we can probably change the code to eliminate the `LAST` field entirely, and just use the `CURRENT` field, if it exists. If we don't want to change the behavior, then we can revert #1616 and simply do the changes that get rid of some boilerplate by using Ample.
Does all this make sense, @Manno15
If not, feel free to give me a call or ping me on Slack.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r489352204
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java
##########
@@ -58,57 +54,31 @@ 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 {
+ try (var tabletsMutator = ample.mutateTablets()) {
for (Assignment assignment : assignments) {
- Mutation m = new Mutation(assignment.tablet.toMetaRow());
- assignment.server.putLocation(m);
- assignment.server.clearFutureLocation(m);
- SuspendingTServer.clearSuspension(m);
- writer.addMutation(m);
+ TabletMutator tabletMutator = tabletsMutator.mutateTablet(assignment.tablet);
+ tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ tabletMutator.deleteLocation(assignment.server, LocationType.FUTURE);
+ tabletMutator.mutate();
Review comment:
Yes, that was an oversight on my part. Added it too it and I will test how that looks.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-689674473
The IT's passed. I merged in the most recent changes to accumulo so hopefully everything is aligned. Most of these things are refactors and deletions of unused code. Not sure how wanted some of these things are so there is no rush for review.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r486974691
##########
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.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r487154328
##########
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:
That's fine. It just needs a comment in the code.
##########
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 if it should be added to other locations. Probably not. I just know it is a behavior change to not handle the RuntimeExceptions that are thrown. I'm okay with that behavior change if it's justified (like, because we know for sure that we actually don't want to catch them here). I'm just not sure whether it's justified.
##########
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:
Well, the reason the ZooTabletStateStore only accepted one was because it can only ever have one... because the root table that uses the ZooTabletStateStore can never have more than one tablet, and therefore, there can never be more than one assignment being updated. That's not true for the other stores, because the metadata table can split into multiple tablets, and so can user tables.
##########
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:
Okay, that makes sense. I tried to trace the callers to this method, to see if they instead ended up reaching the new code path in #1616, but got tired last night. I'll look at it again today. I just want to make sure that if we're not updating it here, that it's because we definitely don't need to.
##########
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:
Based on the code you changed, it does not appear that's the case. Specifically, where you made this update in a loop, it wasn't one at a time. I think you could probably have both the single-case version, and the collection version, so we don't have to do "Collections.singletonList" everywhere when we only have one. The Collection one could be optimized to reuse the same batchwriter somehow (not sure exactly what would need to change in the tablet mutator / Ample code to do that; the old code that didn't use Ample was obviously reusing a batch writer, but I'm not sure how to do it in an Ample way). I'm also not sure how you'd pass the previous location to clear if you're passing a collection... you might have a different previous location for each assignment, right? I'm not sure without analyzing the code in a lot more detail.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r488753208
##########
File path: server/base/src/main/java/org/apache/accumulo/server/metadata/TabletMutatorBase.java
##########
@@ -202,6 +203,23 @@ private String getLocationFamily(LocationType type) {
return this;
}
+ @Override
+ public Ample.TabletMutator putSuspension(Ample.TServer tServer, long suspensionTime) {
+ Preconditions.checkState(updatesEnabled, "Cannot make updates after calling mutate.");
+ mutation.put(SuspendLocationColumn.SUSPEND_COLUMN.getColumnFamily(),
+ SuspendLocationColumn.SUSPEND_COLUMN.getColumnQualifier(),
+ new Value(tServer + "|" + suspensionTime));
Review comment:
Thanks for pointing that out, that was something I didn't notice. I replaced with ` new Value(tServer.getLocation() + "|" + suspensionTime))` which more accurately matches `HostAndPort.toString()`. This also solves the issue I was facing with `MasterRepairsDualAssignmentIT` so I no longer need the if statement I added in the `HostAndPort` file.
I don't fully understand your second paragraph. Are you suggesting I use the `SuspendingTServer.fromValue()` function in this mutation instead of doing the `new Value(tServer.getLocation() + "|" + suspensionTime))`?
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r486977002
##########
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.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r489352204
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java
##########
@@ -58,57 +54,31 @@ 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 {
+ try (var tabletsMutator = ample.mutateTablets()) {
for (Assignment assignment : assignments) {
- Mutation m = new Mutation(assignment.tablet.toMetaRow());
- assignment.server.putLocation(m);
- assignment.server.clearFutureLocation(m);
- SuspendingTServer.clearSuspension(m);
- writer.addMutation(m);
+ TabletMutator tabletMutator = tabletsMutator.mutateTablet(assignment.tablet);
+ tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ tabletMutator.deleteLocation(assignment.server, LocationType.FUTURE);
+ tabletMutator.mutate();
Review comment:
Yes, that was an oversight on my part. Added it back to it and I will test how the one-liner gets formatted.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-691387940
> @ctubbsii Yes, that makes sense. I misunderstood the intent when I was first working on this, my apologies. Thank you for taking a deeper look into it. I would like to set up a call for Monday if you are available. We can discuss how to proceed with both these issues. Again, my apologies.
No worries. It took me awhile to catch on to what was going on, because I didn't know what this `LAST` field was for until today. Even now, I'm only 90% certain. :smiley_cat: Monday would be great. Have a good weekend!
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
Manno15 commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-691281245
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r486977002
##########
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.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-691252150
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r488770528
##########
File path: server/base/src/main/java/org/apache/accumulo/server/metadata/TabletMutatorBase.java
##########
@@ -202,6 +203,23 @@ private String getLocationFamily(LocationType type) {
return this;
}
+ @Override
+ public Ample.TabletMutator putSuspension(Ample.TServer tServer, long suspensionTime) {
+ Preconditions.checkState(updatesEnabled, "Cannot make updates after calling mutate.");
+ mutation.put(SuspendLocationColumn.SUSPEND_COLUMN.getColumnFamily(),
+ SuspendLocationColumn.SUSPEND_COLUMN.getColumnQualifier(),
+ new Value(tServer + "|" + suspensionTime));
Review comment:
> I don't fully understand your second paragraph. Are you suggesting I use the `SuspendingTServer.fromValue()` function in this mutation instead of doing the `new Value(tServer.getLocation() + "|" + suspensionTime))`?
Prior to your changes, the `SuspendingTServer.toValue()` was responsible for creating this `new Value`. I'm suggesting that we leave that class responsible for creating the `new Value`, so it can live alongside that class' existing `fromValue` method. In order to do that, you would need to turn it into a static method, so you can use it in your code.
In other words, this code should change to something like:
```suggestion
SuspendingTServer.toValue(tServer, suspensionTime);
```
And then that method should look something like:
```java
public static Value toValue(HostAndPort tServer, long suspensionTime) {
return new Value(tServer + "|" + suspensionTime);
}
```
I probably don't have the types correct in this code, but something like that is what I'm suggesting.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r488999796
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java
##########
@@ -128,73 +98,56 @@ public void suspend(Collection<TabletLocationState> tablets,
private void unassign(Collection<TabletLocationState> tablets,
Map<TServerInstance,List<Path>> logsForDeadServers, long suspensionTimestamp)
throws DistributedStoreException {
- BatchWriter writer = createBatchWriter();
- try {
+ try (var tabletsMutator = ample.mutateTablets()) {
for (TabletLocationState tls : tablets) {
- Mutation m = new Mutation(tls.extent.toMetaRow());
+ TabletMutator tabletMutator = tabletsMutator.mutateTablet(tls.extent);
if (tls.current != null) {
- tls.current.clearLocation(m);
+ tabletMutator.deleteLocation(tls.current, LocationType.CURRENT);
if (logsForDeadServers != null) {
List<Path> logs = logsForDeadServers.get(tls.current);
if (logs != null) {
for (Path log : logs) {
LogEntry entry =
new LogEntry(tls.extent, 0, tls.current.hostPort(), log.toString());
- m.put(entry.getColumnFamily(), entry.getColumnQualifier(), entry.getValue());
+ tabletMutator.putWal(entry);
}
}
}
if (suspensionTimestamp >= 0) {
- SuspendingTServer suspender =
- new SuspendingTServer(tls.current.getLocation(), suspensionTimestamp);
- suspender.setSuspension(m);
+ tabletMutator.putSuspension(tls.current, suspensionTimestamp);
}
}
if (tls.suspend != null && suspensionTimestamp < 0) {
- SuspendingTServer.clearSuspension(m);
+ tabletMutator.deleteSuspension();
}
if (tls.future != null) {
- tls.future.clearFutureLocation(m);
+ tabletMutator.deleteLocation(tls.future, LocationType.FUTURE);
}
- writer.addMutation(m);
+ tabletMutator.mutate();
}
} catch (Exception ex) {
Review comment:
If all of these code blocks that were updated to use the Ample mutator utility don't throw any checked exceptions, these catch blocks can be updated to only catch the more specific, `RuntimeException`, as in:
```suggestion
} catch (RuntimeException ex) {
```
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java
##########
@@ -58,57 +54,31 @@ 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 {
+ try (var tabletsMutator = ample.mutateTablets()) {
for (Assignment assignment : assignments) {
- Mutation m = new Mutation(assignment.tablet.toMetaRow());
- assignment.server.putLocation(m);
- assignment.server.clearFutureLocation(m);
- SuspendingTServer.clearSuspension(m);
- writer.addMutation(m);
+ TabletMutator tabletMutator = tabletsMutator.mutateTablet(assignment.tablet);
+ tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ tabletMutator.deleteLocation(assignment.server, LocationType.FUTURE);
+ tabletMutator.mutate();
Review comment:
Did you forget to include the `deleteSuspension()` here?
Also, the code might look cleaner if you chained the methods, something like (not formatted):
```suggestion
tabletsMutator.mutateTablet(assignment.tablet)
.putLocation(assignment.server, LocationType.CURRENT)
.deleteLocation(assignment.server, LocationType.FUTURE)
.deleteSuspension()
.mutate();
```
If the formatter makes it look too ugly, you could prevent the formatter from messing with it by wrapping it with a comment line before and after, `// @formatter:off` and `// @formatter:on`.
This kind of simplification can be done in other places in this PR also (I made a review comment elsewhere where it can be a one-liner.
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java
##########
@@ -128,73 +98,56 @@ public void suspend(Collection<TabletLocationState> tablets,
private void unassign(Collection<TabletLocationState> tablets,
Map<TServerInstance,List<Path>> logsForDeadServers, long suspensionTimestamp)
throws DistributedStoreException {
- BatchWriter writer = createBatchWriter();
- try {
+ try (var tabletsMutator = ample.mutateTablets()) {
for (TabletLocationState tls : tablets) {
- Mutation m = new Mutation(tls.extent.toMetaRow());
+ TabletMutator tabletMutator = tabletsMutator.mutateTablet(tls.extent);
if (tls.current != null) {
- tls.current.clearLocation(m);
+ tabletMutator.deleteLocation(tls.current, LocationType.CURRENT);
if (logsForDeadServers != null) {
List<Path> logs = logsForDeadServers.get(tls.current);
if (logs != null) {
for (Path log : logs) {
LogEntry entry =
new LogEntry(tls.extent, 0, tls.current.hostPort(), log.toString());
- m.put(entry.getColumnFamily(), entry.getColumnQualifier(), entry.getValue());
+ tabletMutator.putWal(entry);
}
}
}
if (suspensionTimestamp >= 0) {
- SuspendingTServer suspender =
- new SuspendingTServer(tls.current.getLocation(), suspensionTimestamp);
- suspender.setSuspension(m);
+ tabletMutator.putSuspension(tls.current, suspensionTimestamp);
}
}
if (tls.suspend != null && suspensionTimestamp < 0) {
- SuspendingTServer.clearSuspension(m);
+ tabletMutator.deleteSuspension();
}
if (tls.future != null) {
- tls.future.clearFutureLocation(m);
+ tabletMutator.deleteLocation(tls.future, LocationType.FUTURE);
}
- writer.addMutation(m);
+ tabletMutator.mutate();
}
} catch (Exception ex) {
throw new DistributedStoreException(ex);
- } finally {
- try {
- writer.close();
- } catch (MutationsRejectedException e) {
- throw new DistributedStoreException(e);
- }
}
}
@Override
public void unsuspend(Collection<TabletLocationState> tablets) throws DistributedStoreException {
- BatchWriter writer = createBatchWriter();
- try {
+ try (var tabletsMutator = ample.mutateTablets()) {
for (TabletLocationState tls : tablets) {
if (tls.suspend != null) {
continue;
}
- Mutation m = new Mutation(tls.extent.toMetaRow());
- SuspendingTServer.clearSuspension(m);
- writer.addMutation(m);
+ TabletMutator tabletMutator = tabletsMutator.mutateTablet(tls.extent);
+ tabletMutator.deleteSuspension();
+ tabletMutator.mutate();
Review comment:
```suggestion
tabletsMutator.mutateTablet(tls.extent).deleteSuspension().mutate();
```
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
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.
##########
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.
##########
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.
##########
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.
##########
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r487157843
##########
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 if it should be added to other locations. Probably not. I just know it is a behavior change to not handle the RuntimeExceptions that are thrown. I'm okay with that behavior change if it's justified (like, because we know for sure that we actually don't want to catch them here). I'm just not sure whether it's justified.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#issuecomment-691252150
@Manno15 I updated the title to reflect what this code is trying to change.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r487154328
##########
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:
That's fine. It just needs a comment in the code.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r486976328
##########
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.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
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.
##########
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.
##########
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:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii merged pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
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.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r489363121
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java
##########
@@ -58,57 +54,31 @@ 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 {
+ try (var tabletsMutator = ample.mutateTablets()) {
for (Assignment assignment : assignments) {
- Mutation m = new Mutation(assignment.tablet.toMetaRow());
- assignment.server.putLocation(m);
- assignment.server.clearFutureLocation(m);
- SuspendingTServer.clearSuspension(m);
- writer.addMutation(m);
+ TabletMutator tabletMutator = tabletsMutator.mutateTablet(assignment.tablet);
+ tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ tabletMutator.deleteLocation(assignment.server, LocationType.FUTURE);
+ tabletMutator.mutate();
Review comment:
I'm going to go ahead and chain the methods in the other state stores and various files that use tabletMutator.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r486976328
##########
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.
----------------------------------------------------------------
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #1653: Use Ample for tablet location updates
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r488680360
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/state/MetaDataStateStore.java
##########
@@ -58,57 +54,31 @@ 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.clearFutureLocation(m);
- SuspendingTServer.clearSuspension(m);
- writer.addMutation(m);
+ TabletMutator tabletMutator = ample.mutateTablet(assignment.tablet);
+ tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ tabletMutator.deleteLocation(assignment.server, LocationType.FUTURE);
+ tabletMutator.mutate();
}
} catch (Exception ex) {
throw new DistributedStoreException(ex);
- } finally {
- try {
- writer.close();
- } catch (MutationsRejectedException e) {
- throw new DistributedStoreException(e);
- }
- }
- }
-
- BatchWriter createBatchWriter() {
- try {
- return context.createBatchWriter(targetTableName,
- new BatchWriterConfig().setMaxMemory(MAX_MEMORY)
- .setMaxLatency(LATENCY, TimeUnit.MILLISECONDS).setMaxWriteThreads(THREADS));
- } catch (Exception e) {
- throw new RuntimeException(e);
}
}
@Override
public void setFutureLocations(Collection<Assignment> assignments)
throws DistributedStoreException {
- BatchWriter writer = createBatchWriter();
try {
for (Assignment assignment : assignments) {
- Mutation m = new Mutation(assignment.tablet.toMetaRow());
- SuspendingTServer.clearSuspension(m);
- assignment.server.putFutureLocation(m);
- writer.addMutation(m);
+ TabletMutator tabletMutator = ample.mutateTablet(assignment.tablet);
+ tabletMutator.deleteSuspension();
+ tabletMutator.putLocation(assignment.server, LocationType.FUTURE);
+ tabletMutator.mutate();
Review comment:
I think this loop could benefit from the same batch writer reuse that you did in the unassign method below this, where you called `try (var tabletsMutator = ample.mutateTablets()) {` outside the loop. (Same comment applies to the `setLocations` method above.)
##########
File path: server/base/src/main/java/org/apache/accumulo/server/metadata/TabletMutatorBase.java
##########
@@ -202,6 +203,23 @@ private String getLocationFamily(LocationType type) {
return this;
}
+ @Override
+ public Ample.TabletMutator putSuspension(Ample.TServer tServer, long suspensionTime) {
+ Preconditions.checkState(updatesEnabled, "Cannot make updates after calling mutate.");
+ mutation.put(SuspendLocationColumn.SUSPEND_COLUMN.getColumnFamily(),
+ SuspendLocationColumn.SUSPEND_COLUMN.getColumnQualifier(),
+ new Value(tServer + "|" + suspensionTime));
Review comment:
This replaces `SuspendingTServer.toValue()`, which does not appear to be used any longer. The old method stored a `HostAndPort.toString()`, but this stores `Ample.TServer.toString()`. Are these identical?
The serialization for `toValue` should probably remain in the `SuspendingTServer` class, since that's where the corresponding `fromValue` method is. So, I recommend changing `SuspendingTServer.toValue` that is called from here, so if the serialization changes, it can be changed in both `toValue` and `fromValue` at the same time.
----------------------------------------------------------------
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
[GitHub] [accumulo] Manno15 commented on a change in pull request #1653: WIP. Previously Reverted Code with Fix
Posted by GitBox <gi...@apache.org>.
Manno15 commented on a change in pull request #1653:
URL: https://github.com/apache/accumulo/pull/1653#discussion_r486987280
##########
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.
----------------------------------------------------------------
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