Moved ThriftClientHandler out of TabletServer for #1581

[andrewglowacki]

Moved ThriftClientHandler into it's own file out of TabletServer. Also moved AssignmentHandler and UnloadTabletHandler into separate files (from TabletServer) since they are created by ThriftClientHandler and can no longer be inner classes.

ThriftClientHandler is still tightly coupled to TabletServer and can't really exist on it's own.

[ctubbsii]

Thanks for the contribution, @andrewglowacki !
Overall, the changes look good. However, I made a few comments and suggestions throughout.

One request: please don't force push to your branch, as it will make it difficult to see incremental changes. This is especially important given how much code was shuffled around. Instead, you can just add new commits to your branch. I can squash the changes down to one commit when I merge it in.

[andrewglowacki]

Thanks for the detailed review and great comments!

Unfortunately, while I was making changes, I rebased master onto my branch (to get my vscode changes), and it re-wrote about 4-5 commits from master by mistake. I backed those out, but I had to force push since I had pushed them to my remote branch.

Sorry to cause problems - I'm still getting used to using GitHub like this. Everything looks to be the way it was, but let me know if you see otherwise.

[ctubbsii]

Merging master onto your branch would have been okay if you need to pick up changes from there. It's just the force-push that breaks things... it causes all review comments to become automatically obsolete, and it makes it extremely difficult, if not impossible, to view what changed from the last code review. So, a code reviewer basically has to start over, reviewing from scratch.

[andrewglowacki]

Good to know next time, sorry again! I didn't change the original commit, so all of the comments should still be applicable.

[ctubbsii]

Good to know next time, sorry again! I didn't change the original commit, so all of the comments should still be applicable.

No worries.

To be honest, I used to prefer force pushing a lot... and didn't understand why it mattered that much when others, such as @keith-turner, expressed a preference to avoid it. Then, I started doing a lot more code reviews, and then it became a pain point. So now I've become one of the ones advising people not to force push. Tables turned 😺

[keith-turner]

@andrewglowacki would you be able to highlight the places where these changes may be doing something different or new?

[andrewglowacki]

@keith-turner sorry, what changes do you mean? The force push didn't add/remove anything from the original commit, it just removed the commits that I accidently pulled in when I rebased master. This is what happened:

1. Original commit: 8d99cdc
2. Reviewed by @ctubbsii
3. Rebased master which pulled in and modified the next 4 commits
4. Pushed this to my remote branch before I realized the 4 commits were modified by the rebase
5. Reset the head to 8d99cdc
6. Force pushed the branch to remove the 4 commits
7. Committed and pushed my changes from the review in 1bf3ba5

[keith-turner]

@andrewglowacki this was my first time to look at the PR this morning, so the force push did not impact me. I think a lot of the changes are only something like adding server. with no other changes to code. I was just wondering if you could point me to more substantial changes to review, like a changing a foreach loop to a java stream. One possible way to do this is to comment on the PR where these more substantial changes are. I am just looking for a way to review this more quickly, but I don't want to take much more of your time. So if this would not be quick for you, don't worry about it.

[andrewglowacki]

@keith-turner I see, great! No problem, I don't mind pointing things out. I'll do anything to make it easier to review, especially since it's such a big change.

Unfortunately there's quite a few changes, however virtually all of them are either:

• prepending variables that were owned by TabletServer with 'server.' such as:
  sessionManager.unreserveSession(scanSession);
  to:
  server.sessionManager.unreserveSession(scanSession);

• prepending methods that were owned by TabletServer with 'server.' such as:
  long totalQueued = updateTotalQueuedMutationSize(additionalMutationSize);
  to:
  long totalQueued = server.updateTotalQueuedMutationSize(additionalMutationSize);

• changing private variables to package-private in TabletServer.java such as:
  private final TabletServerResourceManager resourceManager;
  to:
  final TabletServerResourceManager resourceManager;

In addition to making these types of changes to ThriftClientHandler, they were also necessary in AssignmentHandler and UnloadTabletHandler for the same reasons.

[keith-turner]

@andrewglowacki the three changes you just mentioned are great and I approve of those. I was just looking for pointers to anything else you may know of. I plan to take a look over the changes this afternoon.

[ctubbsii]

@andrewglowacki I will try to take a look at this Tuesday, and merge it in if I don't see any issues with it. I gave myself a break for the weekend. 😺

[andrewglowacki]

@ctubbsii no problem, thanks!

[andrewglowacki]

@keith-turner thanks for the review! I will make these changes.

[keith-turner]

@keith-turner thanks for the review! I will make these changes.

No problem. I did the refactoring locally in Eclipse and then compared that to your branch. That made the diff much smaller and allowed me to see all of the changes you made that were not automatic.

[andrewglowacki]

I did the refactoring locally in Eclipse and then compared that to your branch. That made the diff much smaller and allowed me to see all of the changes you made that were not automatic.

Good idea! Yes, it was pretty difficult to discern otherwise.

[ctubbsii]

Everything looks good to me, @andrewglowacki ; I made a couple very trivial changes, which were easier to add as an extra commit on top of yours, rather than type up as feedback.

I'm going to wait for a full build overnight, then merge tomorrow if there's no issues.

[ctubbsii]

All tests passed (well, SuspendedTabletsIT.crashAndResumeTserver
experienced an unrelated timeout, but passed when I re-ran it; it's probably transient, performance related on the build system). I'm merging this.

[ctubbsii]

Thanks for the PR @andrewglowacki ; just to reiterate a previous comment: #1583 (comment)

[andrewglowacki]

No problem. Thanks for the reviews and help!