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