You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "cheelio (via GitHub)" <gi...@apache.org> on 2023/08/28 14:10:20 UTC
[GitHub] [accumulo] cheelio opened a new pull request, #3730: Fix TabletServer shutdown
cheelio opened a new pull request, #3730:
URL: https://github.com/apache/accumulo/pull/3730
Fixes #3282
My attempt to fix the Tabletserver shutdown code.
* Basically I stripped out the `shutdownComplete` code from `TabletServer` because the `shutdownComplete` variable was never set.
* Further the `halt` method in `TabletClientHandler` seemed to cause unwanted behaviour because it prematurely exits the JVM before shutdown was handled. I enterily removed the `Halt.halt()` from that method.
* In `TabletServer` I think that the Halt.halt call in `LockWactcher`'s lostLock method is placed incorrecly. I guess it makes more sense putting it inside the `!serverStopRequested` if statement.
* After making these changes, the shutdown code is executed correctly but that also caused the `tabletServerLock.unlock()` was called twice, causing a NullPointerException when trying to unlock for the second time. I am not entirely sure weather one of these `unlock` calls can be 'dropped'.
--
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] EdColeman commented on pull request #3730: Fix TabletServer shutdown
Posted by "EdColeman (via GitHub)" <gi...@apache.org>.
EdColeman commented on PR #3730:
URL: https://github.com/apache/accumulo/pull/3730#issuecomment-1695782503
Did you intend to target this for 3.1 (current main) or as a fix to 2.1.3? You may need to adjust the target branch and resolve changes for 2.1.3.
--
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] ctubbsii commented on pull request #3730: Fix TabletServer shutdown
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3730:
URL: https://github.com/apache/accumulo/pull/3730#issuecomment-1698074719
> Did you intend to target this for 3.1 (current main) or as a fix to 2.1.3? You may need to adjust the target branch and resolve changes for 2.1.3.
This is not an urgent issue for 2.1 and not worth any behavior changes there. I wouldn't suggest targeting that branch. The main branch should suffice, pending the outcome of code reviews.
--
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] dlmarion commented on a diff in pull request #3730: Fix TabletServer shutdown
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #3730:
URL: https://github.com/apache/accumulo/pull/3730#discussion_r1307715223
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletClientHandler.java:
##########
@@ -1214,16 +1214,14 @@ public void halt(TInfo tinfo, TCredentials credentials, String lock)
checkPermission(security, context, server, credentials, lock, "halt");
- Halt.halt(0, () -> {
Review Comment:
This is called from `fastHalt` below and removing the call to `Halt` here would be a behavior change as it would not stop the tserver immediately. Not sure why we have two methods that do the same thing, likely they were different at some point.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java:
##########
@@ -879,22 +878,6 @@ public void run() {
}
}
- // wait for shutdown
- // if the main thread exits oldServer the manager listener, the JVM will
- // kill the other threads and finalize objects. We want the shutdown that is
- // running in the manager listener thread to complete oldServer this happens.
- // consider making other threads daemon threads so that objects don't
- // get prematurely finalized
- synchronized (this) {
- while (!shutdownComplete) {
- try {
- this.wait(1000);
- } catch (InterruptedException e) {
- log.error(e.toString());
- }
- }
- }
Review Comment:
Certainly if `shutdownComplete` is not set, this would cause the tserver to not shutdown normally I think.
--
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