You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "cshannon (via GitHub)" <gi...@apache.org> on 2023/03/24 17:26:43 UTC
[GitHub] [accumulo] cshannon opened a new pull request, #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
cshannon opened a new pull request, #3257:
URL: https://github.com/apache/accumulo/pull/3257
The previous version where Location extends TServerInstance was error prone if euqals/hashcode was overriden. This fixes things by encapsulating TServerInstance and creating proper equals/hashcode methods so comparsions won't fail.
This fixes #3254
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1147980778
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MetaDataStateStore.java:
##########
@@ -62,10 +62,10 @@ public void setLocations(Collection<Assignment> assignments) throws DistributedS
try (var tabletsMutator = ample.mutateTablets()) {
for (Assignment assignment : assignments) {
TabletMutator tabletMutator = tabletsMutator.mutateTablet(assignment.tablet);
- tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ tabletMutator.putLocation(TabletMetadata.Location.current(assignment.server));
Review Comment:
Latest commit changes the imports to clean it up
##########
server/base/src/test/java/org/apache/accumulo/server/manager/state/RootTabletStateStoreTest.java:
##########
@@ -139,8 +140,9 @@ public void testRootTabletStateStore() throws DistributedStoreException {
assertThrows(IllegalArgumentException.class, () -> tstore.setFutureLocations(assignmentList));
try {
- TabletLocationState broken =
- new TabletLocationState(notRoot, server, null, null, null, null, false);
+ TabletLocationState broken = new TabletLocationState(notRoot,
+ new TabletMetadata.Location(server, TabletMetadata.LocationType.FUTURE), null, null, null,
Review Comment:
Latest commit adds more static methods and makes the constructors private
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1147962280
##########
core/src/main/java/org/apache/accumulo/core/metadata/TabletLocationState.java:
##########
@@ -119,15 +120,23 @@ public boolean hasSuspend() {
public TabletState getState(Set<TServerInstance> liveServers) {
if (hasFuture()) {
- return liveServers.contains(future) ? TabletState.ASSIGNED
+ return liveServers.contains(future.getServerInstance()) ? TabletState.ASSIGNED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasCurrent()) {
- return liveServers.contains(current) ? TabletState.HOSTED
+ return liveServers.contains(current.getServerInstance()) ? TabletState.HOSTED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasSuspend()) {
return TabletState.SUSPENDED;
} else {
return TabletState.UNASSIGNED;
}
}
+
+ private static TabletMetadata.Location validateLocation(TabletMetadata.Location location,
+ TabletMetadata.LocationType type) {
+ if (location != null && !location.getType().equals(type)) {
+ throw new IllegalArgumentException("Location type is required to be of type " + type);
+ }
+ return location;
+ }
Review Comment:
~~There are two constructors, would this mean 6 static methods are needed? Six static methods seems fine. Making the constructors private if it makes the code shorter and more consistent SGTM.~~
Made this comment in the wrong place. Copied it to the correct 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1147962280
##########
core/src/main/java/org/apache/accumulo/core/metadata/TabletLocationState.java:
##########
@@ -119,15 +120,23 @@ public boolean hasSuspend() {
public TabletState getState(Set<TServerInstance> liveServers) {
if (hasFuture()) {
- return liveServers.contains(future) ? TabletState.ASSIGNED
+ return liveServers.contains(future.getServerInstance()) ? TabletState.ASSIGNED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasCurrent()) {
- return liveServers.contains(current) ? TabletState.HOSTED
+ return liveServers.contains(current.getServerInstance()) ? TabletState.HOSTED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasSuspend()) {
return TabletState.SUSPENDED;
} else {
return TabletState.UNASSIGNED;
}
}
+
+ private static TabletMetadata.Location validateLocation(TabletMetadata.Location location,
+ TabletMetadata.LocationType type) {
+ if (location != null && !location.getType().equals(type)) {
+ throw new IllegalArgumentException("Location type is required to be of type " + type);
+ }
+ return location;
+ }
Review Comment:
There are two constructors, would this mean 6 static methods are needed? Six static methods seems fine. Making the constructors private if it makes the code shorter and more consistent SGTM.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#issuecomment-1483174285
@keith-turner - I went ahead and targeted this for 2.1.1 since it's not public APIs but that could be changed. I updated the Ample interface to pass in a Location instead of the tserver and location type. If we want we could keep backwards compatibility by adding some default methods that delegate to the new methods, for example:
```java
default T putLocation(TServerInstance tserver, TabletMetadata.LocationType type) {
return putLocation(new TabletMetadata.Location(tserver, type));
}
default T deleteLocation(TServerInstance tserver, TabletMetadata.LocationType type) {
return putLocation(new TabletMetadata.Location(tserver, type));
}
```
But I did not do that for now as I figured it would be cleaner to just change the existing. Also, I didn't add any new tests because all of the existing tests seem to have enough coverage. I ran through the Sunny ITs and everything passed and I can kick off a full IT. I also grabbed your branch from #3251 locally and merged this in and after fixing the merge conflicts I verified the TabletMetadataTest still passed with the new update.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1147963081
##########
server/base/src/test/java/org/apache/accumulo/server/manager/state/RootTabletStateStoreTest.java:
##########
@@ -139,8 +140,9 @@ public void testRootTabletStateStore() throws DistributedStoreException {
assertThrows(IllegalArgumentException.class, () -> tstore.setFutureLocations(assignmentList));
try {
- TabletLocationState broken =
- new TabletLocationState(notRoot, server, null, null, null, null, false);
+ TabletLocationState broken = new TabletLocationState(notRoot,
+ new TabletMetadata.Location(server, TabletMetadata.LocationType.FUTURE), null, null, null,
Review Comment:
There are two constructors, would this mean 6 static methods are needed? Six static methods seems fine. Making the constructors private if it makes the code shorter and more consistent SGTM.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1147956432
##########
core/src/main/java/org/apache/accumulo/core/metadata/TabletLocationState.java:
##########
@@ -119,15 +120,23 @@ public boolean hasSuspend() {
public TabletState getState(Set<TServerInstance> liveServers) {
if (hasFuture()) {
- return liveServers.contains(future) ? TabletState.ASSIGNED
+ return liveServers.contains(future.getServerInstance()) ? TabletState.ASSIGNED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasCurrent()) {
- return liveServers.contains(current) ? TabletState.HOSTED
+ return liveServers.contains(current.getServerInstance()) ? TabletState.HOSTED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasSuspend()) {
return TabletState.SUSPENDED;
} else {
return TabletState.UNASSIGNED;
}
}
+
+ private static TabletMetadata.Location validateLocation(TabletMetadata.Location location,
+ TabletMetadata.LocationType type) {
+ if (location != null && !location.getType().equals(type)) {
+ throw new IllegalArgumentException("Location type is required to be of type " + type);
+ }
+ return location;
+ }
Review Comment:
Looking through the PR it seems like TabletGroupWatcher would be the main beneficiary of keep the current types, but not sure what class would not benefit.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1147921480
##########
core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java:
##########
@@ -133,17 +133,77 @@ public enum ColumnType {
ECOMP
}
- public static class Location extends TServerInstance {
+ public static class Location {
+ private final TServerInstance tServerInstance;
private final LocationType lt;
- public Location(String server, String session, LocationType lt) {
- super(HostAndPort.fromString(server), session);
- this.lt = lt;
+ public Location(final String server, final String session, final LocationType lt) {
+ this(new TServerInstance(HostAndPort.fromString(server), session), lt);
+ }
+
+ public Location(final TServerInstance tServerInstance, final LocationType lt) {
+ this.tServerInstance =
+ Objects.requireNonNull(tServerInstance, "tServerInstance must not be null");
+ this.lt = Objects.requireNonNull(lt, "locationType must not be null");
Review Comment:
adding the not null check is nice
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MetaDataStateStore.java:
##########
@@ -62,10 +62,10 @@ public void setLocations(Collection<Assignment> assignments) throws DistributedS
try (var tabletsMutator = ample.mutateTablets()) {
for (Assignment assignment : assignments) {
TabletMutator tabletMutator = tabletsMutator.mutateTablet(assignment.tablet);
- tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ tabletMutator.putLocation(TabletMetadata.Location.current(assignment.server));
Review Comment:
Could shorten this a bit by importing Location. This is a stylistic comment feel free to ignore it.
```suggestion
tabletMutator.putLocation(Location.current(assignment.server));
```
I like these static methods, I think they shorten the code w/o any loss of readability.
##########
core/src/main/java/org/apache/accumulo/core/metadata/TabletLocationState.java:
##########
@@ -119,15 +120,23 @@ public boolean hasSuspend() {
public TabletState getState(Set<TServerInstance> liveServers) {
if (hasFuture()) {
- return liveServers.contains(future) ? TabletState.ASSIGNED
+ return liveServers.contains(future.getServerInstance()) ? TabletState.ASSIGNED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasCurrent()) {
- return liveServers.contains(current) ? TabletState.HOSTED
+ return liveServers.contains(current.getServerInstance()) ? TabletState.HOSTED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasSuspend()) {
return TabletState.SUSPENDED;
} else {
return TabletState.UNASSIGNED;
}
}
+
+ private static TabletMetadata.Location validateLocation(TabletMetadata.Location location,
+ TabletMetadata.LocationType type) {
+ if (location != null && !location.getType().equals(type)) {
+ throw new IllegalArgumentException("Location type is required to be of type " + type);
+ }
+ return location;
+ }
Review Comment:
Wondering if the future,current, and last instance var should keep the type TServerInstance. It depends on what the most frequent use is, which I am not sure of. I saw one class while looking over this that was always converting these fields to TserverInstance. If it were changed back, could still use this nice validation method with a slight change.
```suggestion
private static TserverInstance validateLocation(TabletMetadata.Location location,
TabletMetadata.LocationType type) {
if (location != null && !location.getType().equals(type)) {
throw new IllegalArgumentException("Location type is required to be of type " + type);
}
return location.getServerInstance();
}
```
##########
server/base/src/test/java/org/apache/accumulo/server/manager/state/RootTabletStateStoreTest.java:
##########
@@ -139,8 +140,9 @@ public void testRootTabletStateStore() throws DistributedStoreException {
assertThrows(IllegalArgumentException.class, () -> tstore.setFutureLocations(assignmentList));
try {
- TabletLocationState broken =
- new TabletLocationState(notRoot, server, null, null, null, null, false);
+ TabletLocationState broken = new TabletLocationState(notRoot,
+ new TabletMetadata.Location(server, TabletMetadata.LocationType.FUTURE), null, null, null,
Review Comment:
```suggestion
new TabletMetadata.Location.future(server), null, null, null,
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#issuecomment-1483278472
> But I did not do that for now as I figured it would be cleaner to just change the existing
I agree with that.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#issuecomment-1483808763
The full IT build passed except for 3 tests (AssignLocationModeIT, CompactLocationModeIT, ManagerAssignmentIT) but this was a good thing as the test themselves needed fixing and the tests breaking showed the new equals/hashcode methods are in fact implemented correctly in Location.
The tests were comparing that the server instances were the same between different states (current, last, etc) and now that we use Location and compare location type the equals failed because the server instances were the same but the LocationTypes were different of course.
Here is how the current comparsion is in main where it's comparing the server instance types: https://github.com/apache/accumulo/blob/ecbff2e6023240f6aaf0a231aff5b88d5dc92ab2/test/src/main/java/org/apache/accumulo/test/functional/AssignLocationModeIT.java#L70
And here is how it's now fixed: https://github.com/apache/accumulo/blob/7a3caf8ac962392083159b1a8ac93a009e75a2fa/test/src/main/java/org/apache/accumulo/test/functional/AssignLocationModeIT.java#L70
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1147956432
##########
core/src/main/java/org/apache/accumulo/core/metadata/TabletLocationState.java:
##########
@@ -119,15 +120,23 @@ public boolean hasSuspend() {
public TabletState getState(Set<TServerInstance> liveServers) {
if (hasFuture()) {
- return liveServers.contains(future) ? TabletState.ASSIGNED
+ return liveServers.contains(future.getServerInstance()) ? TabletState.ASSIGNED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasCurrent()) {
- return liveServers.contains(current) ? TabletState.HOSTED
+ return liveServers.contains(current.getServerInstance()) ? TabletState.HOSTED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasSuspend()) {
return TabletState.SUSPENDED;
} else {
return TabletState.UNASSIGNED;
}
}
+
+ private static TabletMetadata.Location validateLocation(TabletMetadata.Location location,
+ TabletMetadata.LocationType type) {
+ if (location != null && !location.getType().equals(type)) {
+ throw new IllegalArgumentException("Location type is required to be of type " + type);
+ }
+ return location;
+ }
Review Comment:
Looking through the PR it seems like TabletGroupWatcher would be the main beneficiary of keep the current type, but not sure what class would not benefit.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1147957183
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java:
##########
@@ -1405,7 +1405,7 @@ private void waitForTableStateTransition(TableId tableId, TableState expectedSta
lastRow = tablet.getExtent().toMetaRow();
if (loc != null) {
- serverCounts.increment(loc.getHostPortSession(), 1);
+ serverCounts.increment(loc.getServerInstance().getHostPortSession(), 1);
Review Comment:
```suggestion
serverCounts.increment(loc.getHostPortSession(), 1);
```
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1148025480
##########
core/src/main/java/org/apache/accumulo/core/metadata/TabletLocationState.java:
##########
@@ -119,15 +120,23 @@ public boolean hasSuspend() {
public TabletState getState(Set<TServerInstance> liveServers) {
if (hasFuture()) {
- return liveServers.contains(future) ? TabletState.ASSIGNED
+ return liveServers.contains(future.getServerInstance()) ? TabletState.ASSIGNED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasCurrent()) {
- return liveServers.contains(current) ? TabletState.HOSTED
+ return liveServers.contains(current.getServerInstance()) ? TabletState.HOSTED
: TabletState.ASSIGNED_TO_DEAD_SERVER;
} else if (hasSuspend()) {
return TabletState.SUSPENDED;
} else {
return TabletState.UNASSIGNED;
}
}
+
+ private static TabletMetadata.Location validateLocation(TabletMetadata.Location location,
+ TabletMetadata.LocationType type) {
+ if (location != null && !location.getType().equals(type)) {
+ throw new IllegalArgumentException("Location type is required to be of type " + type);
+ }
+ return location;
+ }
Review Comment:
After looking at this more I thought it was still best to keep storing Locations inside of TabletLocationState but I added some helper methods to easily get the TServerInstance out of the current, last or future locations so TabletGroupWatcher is now cleaner and has less changes as it just uses those helper methods when it needs the TServerInstance and not the entire 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1147951907
##########
server/base/src/main/java/org/apache/accumulo/server/manager/state/MetaDataStateStore.java:
##########
@@ -62,10 +62,10 @@ public void setLocations(Collection<Assignment> assignments) throws DistributedS
try (var tabletsMutator = ample.mutateTablets()) {
for (Assignment assignment : assignments) {
TabletMutator tabletMutator = tabletsMutator.mutateTablet(assignment.tablet);
- tabletMutator.putLocation(assignment.server, LocationType.CURRENT);
+ tabletMutator.putLocation(TabletMetadata.Location.current(assignment.server));
Review Comment:
Yeah I agree, it would be even nicer/cleaner if we imported 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1147965228
##########
server/base/src/test/java/org/apache/accumulo/server/manager/state/RootTabletStateStoreTest.java:
##########
@@ -139,8 +140,9 @@ public void testRootTabletStateStore() throws DistributedStoreException {
assertThrows(IllegalArgumentException.class, () -> tstore.setFutureLocations(assignmentList));
try {
- TabletLocationState broken =
- new TabletLocationState(notRoot, server, null, null, null, null, false);
+ TabletLocationState broken = new TabletLocationState(notRoot,
+ new TabletMetadata.Location(server, TabletMetadata.LocationType.FUTURE), null, null, null,
Review Comment:
Yeah, exactly, just to keep it consistent so we use the short static methods everywhere.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on a diff in pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on code in PR #3257:
URL: https://github.com/apache/accumulo/pull/3257#discussion_r1147955160
##########
server/base/src/test/java/org/apache/accumulo/server/manager/state/RootTabletStateStoreTest.java:
##########
@@ -139,8 +140,9 @@ public void testRootTabletStateStore() throws DistributedStoreException {
assertThrows(IllegalArgumentException.class, () -> tstore.setFutureLocations(assignmentList));
try {
- TabletLocationState broken =
- new TabletLocationState(notRoot, server, null, null, null, null, false);
+ TabletLocationState broken = new TabletLocationState(notRoot,
+ new TabletMetadata.Location(server, TabletMetadata.LocationType.FUTURE), null, null, null,
Review Comment:
We don't have to but I could make the constructors protected/private so we don't miss instances of using the static methods like I missed this one.
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon merged pull request #3257: Refactor TabletMetadata.Location to encapsulate TServerInstance
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon merged PR #3257:
URL: https://github.com/apache/accumulo/pull/3257
--
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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org