sourabh912 commented on code in PR #3386:
URL: https://github.com/apache/hive/pull/3386#discussion_r919249214
##########
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:
Any particular reason for removing this check? I think the check can be
modified to
`if (state != STATE.STARTED || state != STATE.DECOMMISSIONED) { throw new
IllegalStateException();}`
##########
service/src/java/org/apache/hive/service/cli/operation/OperationManager.java:
##########
@@ -208,7 +223,10 @@ private String getQueryId(Operation operation) {
return operation.getQueryId();
}
- private void addOperation(Operation operation) {
+ private void addOperation(Operation operation) throws HiveSQLException {
+ if (getServiceState() != STATE.STARTED) {
Review Comment:
I think the better check would be
```
if (state == STATE.DECOMMISSIONING || state == STATE.STOPPED) {
throw new HiveSQLException(ErrorMsg.HIVE_SERVER2_INACTIVE,
state.name());
}
```
##########
service/src/java/org/apache/hive/service/Service.java:
##########
@@ -39,6 +39,12 @@ public enum STATE {
/** started and not stopped */
STARTED,
+ /**
+ * Telling the service not to run new operations from front users,
+ * but existing queries can still be finished normally
+ */
+ DECOMMISSIONED,
Review Comment:
As a user of this interface, `DECOMMISSIONED` and `STOPPED` states are kind
of confusing to me as the states don't define clear boundaries as which state
is expected to do what. I think we should rename DECOMMISSIONED to
DECOMMISSIONING so that a usual state transition of a service would look like:
`STARTED -> DECOMMISSIONING -> STOPPED`
##########
service/src/java/org/apache/hive/service/Service.java:
##########
@@ -63,6 +69,21 @@ public enum STATE {
*/
void start();
+ /**
+ * Imply the service not to run new requests from client. The service acts
accordingly upon
+ * receiving such command. For example:
+ * - HiveServer2: If service discovery is enabled, deregister itself from
Zookeeper and
Review Comment:
We should move the description for HS2 in HS2's decommission api
##########
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) {
Review Comment:
should replace this with
```
zooKeeperHelper != null && !isDeregisteredWithZooKeeper()
```
as it ensures to remove instance from zookeeper if not already removed.
##########
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:
Instead of doing a busy wait which consumes CPU cycles, a better way would
be to handle this via wait/notify mechanism i.e add an api like
`waitForOperationsToFinish(waitTimeOut)` in OperationsManager and call invoke
it here. The implementation of the api would internally wait for operations
count to become zero and notify the caller thread.
--
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]
