sourabh912 commented on code in PR #3386:
URL: https://github.com/apache/hive/pull/3386#discussion_r924864971
##########
service/src/java/org/apache/hive/service/CompositeService.java:
##########
@@ -94,6 +94,19 @@ public synchronized void stop() {
super.stop();
}
+ @Override
+ public synchronized void decommission() {
Review Comment:
nit: add a documentation that services are decommissioned in the reverse
order in which they were added.
##########
service/src/java/org/apache/hive/service/server/HiveServer2.java:
##########
@@ -895,6 +897,60 @@ private void stopOrDisconnectTezSessions() {
}
}
+ /**
+ * Decommission HiveServer2. As a consequence, SessionManager stops
+ * opening new sessions, OperationManager refuses running new queries and
+ * HiveServer2 deregisters itself from Zookeeper if service discovery is
enabled,
+ * but the decommissioning has no effect on the current running queries.
+ */
+ public synchronized void decommission() {
+ LOG.info("Decommissioning HiveServer2");
+ // Remove this server instance from ZooKeeper if dynamic service discovery
is set
+ if (zooKeeperHelper != null && !isDeregisteredWithZooKeeper()) {
+ try {
+ zooKeeperHelper.removeServerInstanceFromZooKeeper();
+ } catch (Exception e) {
+ LOG.error("Error removing znode for this HiveServer2 instance from
ZooKeeper.", e);
+ }
+ }
+ super.decommission();
+ }
+
+ public synchronized void graceful_stop() {
+ try {
+ decommission();
+ long maxTimeForWait = HiveConf.getTimeVar(getHiveConf(),
+ HiveConf.ConfVars.HIVE_SERVER2_GRACEFUL_STOP_TIMEOUT,
TimeUnit.MILLISECONDS);
+ if (maxTimeForWait > 0) {
+ ExecutorService service = Executors.newSingleThreadExecutor();
+ Future future = service.submit(() -> {
+ // For gracefully stopping, sleeping some time while looping does
not bring much overhead,
+ // that is, at most 100ms are wasted for waiting for
OperationManager to be done,
+ // and this code path will only be executed when HS2 is being
terminated.
+ long sleepInterval = Math.min(100, maxTimeForWait);
+ while (getCliService() != null && getCliService().getSessionManager()
+ .getOperations().size() != 0) {
Review Comment:
Should a better check be
```
while(getCliService() != null && getCliService().getSessionManager() != null
&& getCliService().getSessionManager().getOperations().size() > 0) {}
```
##########
service/src/java/org/apache/hive/service/CompositeService.java:
##########
@@ -94,6 +94,19 @@ public synchronized void stop() {
super.stop();
}
+ @Override
+ public synchronized void decommission() {
Review Comment:
Also return early if the service is already decommissioned?
```
if (this.getServiceState() == State.DECOMMISSIONING) {
return
}
```
##########
service/src/java/org/apache/hive/service/server/HiveServer2.java:
##########
@@ -895,6 +897,46 @@ private void stopOrDisconnectTezSessions() {
}
}
+ public synchronized void decommission() {
+ LOG.info("Decommissioning HiveServer2");
+ // Remove this server instance from ZooKeeper if dynamic service discovery
is set
+ if (serviceDiscovery && !activePassiveHA && zooKeeperHelper != null) {
+ try {
+ zooKeeperHelper.removeServerInstanceFromZooKeeper();
+ } catch (Exception e) {
+ LOG.error("Error removing znode for this HiveServer2 instance from
ZooKeeper.", e);
+ }
+ }
+ super.decommission();
+ }
+
+ public synchronized void graceful_stop() {
+ try {
+ decommission();
+ long maxTimeForWait = HiveConf.getTimeVar(getHiveConf(),
+ HiveConf.ConfVars.HIVE_SERVER2_GRACEFUL_STOP_TIMEOUT,
TimeUnit.MILLISECONDS);
+ if (maxTimeForWait > 0) {
+ ExecutorService service = Executors.newSingleThreadExecutor();
+ Future future = service.submit(() -> {
+ while (getCliService() != null && getCliService().getSessionManager()
Review Comment:
Thank you for addressing my comment.
Any particular reason why we need a separate thread to wait on operations to
finish? Why can't it be done in same thread calling `graceful_stop` ?
##########
service/src/java/org/apache/hive/service/AbstractService.java:
##########
@@ -125,7 +145,6 @@ public synchronized void stop() {
// started (eg another service failing canceled startup)
return;
}
- ensureCurrentState(STATE.STARTED);
Review Comment:
Understood. Instead of removing the check, shall we change it to one
mentioned in my previous comment?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
