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/11/10 23:07:37 UTC
[GitHub] [accumulo] milleruntime opened a new pull request #1774: Refactor Tablet state classes
milleruntime opened a new pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774
* Move TabletState, TabletLocationState, TServerInstance and
SuspendingTServer from master to core.metadata package to allow
packages outside of master to use them
* Make ample TabletMetadata.Location extend TServerInstance to
eliminate a temporary interface and some redundant object creation
* Rewrite GroupBalancer getLocationProvider() method to eliminate
Location inner class
* Modify GroupBalancerTest to override new method
----------------------------------------------------------------
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] milleruntime commented on pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#issuecomment-726416569
@ctubbsii @keith-turner Do you think the class TServerInstance needs to be serializable? We have a transient field declared and are overriding the java serilazation methods but I can't find where we are serializing the object. https://github.com/apache/accumulo/blob/bfe173ec140f07086b0ff8940fc08d640b9539a7/server/base/src/main/java/org/apache/accumulo/server/master/state/TServerInstance.java#L45-L46
I am wondering if this was done because the old Guava object that HostAndPort was taken from at some point was not serializable.
https://github.com/apache/accumulo/blob/bfe173ec140f07086b0ff8940fc08d640b9539a7/server/base/src/main/java/org/apache/accumulo/server/master/state/TServerInstance.java#L125-L133
----------------------------------------------------------------
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] milleruntime merged pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
milleruntime merged pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#discussion_r525240571
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/MasterClientServiceHandler.java
##########
@@ -278,7 +278,9 @@ public void shutdownTabletServer(TInfo info, TCredentials c, String tabletServer
log.debug("Seeding FATE op to shutdown " + tabletServer + " with tid " + tid);
- master.fate.seedTransaction(tid, new TraceRepo<>(new ShutdownTServer(doomed, force)), false);
+ master.fate.seedTransaction(tid,
+ new TraceRepo<>(new ShutdownTServer(doomed.getHostAndPort(), doomed.getSession(), force)),
Review comment:
Oh yeah, good idea.
----------------------------------------------------------------
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 edited a comment on pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
ctubbsii edited a comment on pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#issuecomment-728158049
> @ctubbsii @keith-turner Do you think the class TServerInstance needs to be serializable? We have a transient field declared and are overriding the java serilazation methods but I can't find where we are serializing the object.
I found one place where it is serialized as part of a FaTE RepO: `ShutdownTServer` has a member field called `server` of type `TServerInstance`.
However, I did notice that it the customizations we've done to serialize and deserialize the `HostAndPort` transient member is because of the comment that says that type isn't serializable. However, the type we're using (the one copied into our code base from Guava) actually *is* serializable. So, we can probably just remove those customization methods, bump the `serialVersionUid` to `2L` (unless you're relocating it, then it doesn't matter if it stays the same, since it's a new object anyway), and make the `HostAndPort location` field `final` and remove the `transient` keyword.
Changing the serialization would affect previous FaTE operations, but the only one I could find was the `ShutdownTServer` one, and I don't think that would matter across upgrades anyway.
----------------------------------------------------------------
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] milleruntime commented on pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#issuecomment-728347378
> Actually, since `ShutdownTServer` looks like it's the only place where it is ever serialized, I'd probably prefer to drop `Serializable` altogether (still make the HostAndPort field final), and change `ShutdownTServer`'s serialization to use Strings instead. There's no reason we need to maintain serialization compatibility for `TServerLocation` when it's only ever serialized once, out of over 500 references in the code base.
In b735d2291bc59c416c I dropped the serialization of TServerInstance and made `ShutdownTServer` serialize HostAndPort and String instead. I thought this was better since HostAndPort is actually serializable but is not using custom code. Using the HostAndPort in `ShutdownTServer` made it easier to reconstruct `TServerInstance` in the FATE Operation.
----------------------------------------------------------------
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 #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#issuecomment-728158049
> @ctubbsii @keith-turner Do you think the class TServerInstance needs to be serializable? We have a transient field declared and are overriding the java serilazation methods but I can't find where we are serializing the object.
I found one place where it is serialized as part of a FaTE RepO: `ShutdownTServer` has a member field called `server` of type `TServerInstance`.
However, I did notice that it the customizations we've done to serialize and deserialize the `HostAndPort` transient member is because of the comment that says that type isn't serializable. However, the type we're using (the one copied into our code base from Guava) actually *is* serializable. So, we can probably just remove those customization methods, bump the `serialVersionUid` to `2L`, and make the `HostAndPort location` field `final` and remove the `transient` keyword.
----------------------------------------------------------------
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] milleruntime commented on a change in pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#discussion_r522267922
##########
File path: core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java
##########
@@ -145,7 +145,7 @@ public void testFuture() {
EnumSet.allOf(ColumnType.class), false);
assertEquals(extent, tm.getExtent());
- assertEquals(HostAndPort.fromParts("server1", 8555), tm.getLocation().getHostAndPort());
+ assertEquals(HostAndPort.fromParts("server1", 8555), tm.getLocation().getLocation());
Review comment:
Yes. This is holdover from the temp interface in Ample. I thought I had dropped this but I got side tracked. I will drop this interface in the next commit.
----------------------------------------------------------------
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] milleruntime commented on pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#issuecomment-726199653
I also realized that moving `TServerInstance` will most likely break serialization so we may have to do something in the upgrade code to prevent issues upgrading. This class is weird though... there are `writeObject(out)` and `readObject(in)` methods which look similar to Thrift classes and the class starting with "T" make me think that maybe this was based on a thrift class at one point. I had thought the "T" was for Tablet Server.
----------------------------------------------------------------
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 #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#discussion_r525273855
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
##########
@@ -1301,7 +1301,7 @@ private void waitForTableStateTransition(TableId tableId, TableState expectedSta
lastRow = tablet.getExtent().toMetaRow();
if (loc != null) {
- serverCounts.increment(loc.getId(), 1);
+ serverCounts.increment(loc.toString(), 1);
Review comment:
> Maybe something very explicit like `getHostPortSession()` would be better. Then it will be clear what parts of the code we need to change if we drop/change the session.
:+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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] milleruntime commented on a change in pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on a change in pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#discussion_r522266607
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
##########
@@ -1301,7 +1301,7 @@ private void waitForTableStateTransition(TableId tableId, TableState expectedSta
lastRow = tablet.getExtent().toMetaRow();
if (loc != null) {
- serverCounts.increment(loc.getId(), 1);
+ serverCounts.increment(loc.toString(), 1);
Review comment:
They should be returning the same thing, but I don't think either method name is appropriate. Looking at the impl, they both ultimately return `hostPort() + "[" + session + "]"`.
Maybe something very explicit like `getHostPortSession()` would be better. Then it will be clear what parts of the code we need to change if we drop/change the session.
----------------------------------------------------------------
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] milleruntime commented on pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#issuecomment-726175596
> Aside from a few String-based concerns, my main concern with these changes is that "location" means too many different things. In some contexts, it means "a specific tserver instance", but in other contexts the same word "location" means "host address and port". It gets more complicated when we pass a location containing a location, or assign a location variable named loc to some tserver's location's location.
>
> In some places, it's very clear, we just mean "host address and port (as a String)". It might be nice to have that kind of clarity everywhere, instead of trying to figure out which "location" we mean every time we see it. This might just be a matter of choosing better variable and method names where these types are used, to accompany your effort to simplify the types/classes.
I agree. Your concerns with location terminology is prevalent everywhere across our code. These changes were just a first pass, moving the master classes to core and combining the inner classes. I can improve the methods in TServerInstance as well in this PR but I didn't want to make too many changes at once.
----------------------------------------------------------------
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] milleruntime edited a comment on pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
milleruntime edited a comment on pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#issuecomment-726403361
> there are `writeObject(out)` and `readObject(in)` methods
So I think these were added because of the `transient` field. When I removed these, this caused spotbugs to fail. I will fix this.
----------------------------------------------------------------
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] keith-turner commented on pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
keith-turner commented on pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#issuecomment-728691754
> @ctubbsii @keith-turner Do you think the class TServerInstance needs to be serializable?
I would guess some FATE ops use it. Seems like @ctubbsii found one. Hopefully that is the only one and it can be removed.
----------------------------------------------------------------
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 #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#issuecomment-728165175
Actually, since `ShutdownTServer` looks like it's the only place where it is ever serialized, I'd probably prefer to drop `Serializable` altogether (still make the HostAndPort field final), and change `ShutdownTServer`'s serialization to use Strings instead. There's no reason we need to maintain serialization compatibility for `TServerLocation` when it's only ever serialized once, out of over 500 references in the code base.
----------------------------------------------------------------
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 #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#discussion_r524648022
##########
File path: server/manager/src/main/java/org/apache/accumulo/master/MasterClientServiceHandler.java
##########
@@ -278,7 +278,9 @@ public void shutdownTabletServer(TInfo info, TCredentials c, String tabletServer
log.debug("Seeding FATE op to shutdown " + tabletServer + " with tid " + tid);
- master.fate.seedTransaction(tid, new TraceRepo<>(new ShutdownTServer(doomed, force)), false);
+ master.fate.seedTransaction(tid,
+ new TraceRepo<>(new ShutdownTServer(doomed.getHostAndPort(), doomed.getSession(), force)),
Review comment:
The constructor can still use this type, since that won't affect serialization. The fields can be derived from the constructor parameter, rather than passed in as separate parameters.
----------------------------------------------------------------
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 #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#discussion_r521069066
##########
File path: core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java
##########
@@ -145,7 +145,7 @@ public void testFuture() {
EnumSet.allOf(ColumnType.class), false);
assertEquals(extent, tm.getExtent());
- assertEquals(HostAndPort.fromParts("server1", 8555), tm.getLocation().getHostAndPort());
+ assertEquals(HostAndPort.fromParts("server1", 8555), tm.getLocation().getLocation());
Review comment:
`getLocation().getLocation()` seems funny here. Too many types known as 'location'.
##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
##########
@@ -1301,7 +1301,7 @@ private void waitForTableStateTransition(TableId tableId, TableState expectedSta
lastRow = tablet.getExtent().toMetaRow();
if (loc != null) {
- serverCounts.increment(loc.getId(), 1);
+ serverCounts.increment(loc.toString(), 1);
Review comment:
This seems like a regression. In general, unless you're printing something, it's better to use a more specific type than a String type, and a more specific method name than `toString()`, since `toString()` should not be relied upon to not change over 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] milleruntime commented on pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#issuecomment-726288320
@ctubbsii I renamed most of the methods in 6893e01 so the changes in this PR will be OK to stand on their own. I definitely want to do more with these classes and continue to improve their uses, similar to the work I did with TabletFile.
----------------------------------------------------------------
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] milleruntime commented on pull request #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
milleruntime commented on pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#issuecomment-726403361
> there are `writeObject(out)` and `readObject(in)` methods
So I think these were added because of the `transient` field. I will fix this.
----------------------------------------------------------------
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 #1774: Refactor Tablet state classes
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#discussion_r525315192
##########
File path: server/base/src/main/java/org/apache/accumulo/server/master/balancer/GroupBalancer.java
##########
@@ -79,21 +79,13 @@ public GroupBalancer(TableId tableId) {
this.tableId = tableId;
}
- protected Iterable<Pair<KeyExtent,Location>> getLocationProvider() {
- return () -> {
- try {
- return TabletsMetadata.builder().forTable(tableId).fetch(LOCATION, PREV_ROW).build(context)
- .stream().map(tm -> {
- Location loc = Location.NONE;
- if (tm.hasCurrent()) {
- loc = new Location(new TServerInstance(tm.getLocation()));
- }
- return new Pair<>(tm.getExtent(), loc);
- }).iterator();
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
- };
+ protected Map<KeyExtent,TServerInstance> getLocationProvider() {
+ Map<KeyExtent,TServerInstance> tablets = new LinkedHashMap<>();
+ for (var tm : TabletsMetadata.builder().forTable(tableId).fetch(LOCATION, PREV_ROW)
+ .build(context)) {
+ tablets.put(tm.getExtent(), tm.getLocation());
+ }
+ return tablets;
Review comment:
Nice cleanup here.
----------------------------------------------------------------
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