mattcasters commented on code in PR #9179:
URL: https://github.com/apache/hop/pull/9179#discussion_r4435555507
##########
engine/src/main/java/org/apache/hop/lineage/hub/LineageHub.java:
##########
@@ -162,25 +168,30 @@ public void flushQuietly() {
}
}
- /** Stop the worker thread and shut down sinks. */
+ /**
+ * Stop the worker thread and shut down sinks. Calls are serialized: a
second call waits until the
+ * first one has delivered the remaining events, instead of shutting the
sinks down under it.
+ */
public void shutdown() {
- running.set(false);
- Thread t = worker;
- if (t != null) {
- t.interrupt();
- try {
- t.join(5000);
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
+ synchronized (shutdownLock) {
+ running.set(false);
+ Thread t = worker;
+ if (t != null) {
+ t.interrupt();
+ try {
+ t.join(5000);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
}
+ worker = null;
Review Comment:
**[bug]** `shutdown()` holds `shutdownLock`, but `emit()` /
`ensureWorkerStarted()` do not. On SIGTERM the JVM starts every shutdown hook
at once, and `HopEnvironment.shutdown()` calls `LineageHub.shutdown()`
immediately, while `HopRunShutdownHook` is still stopping the pipeline and only
shuts the hub down afterwards. The terminal run event is therefore emitted
during or just after this call.
`running` is already false, so that `emit()` starts a new worker and stores
it in `worker`. This `worker = null` drops that reference. `drainRemaining()` /
`shutdownSinks()` can then close the sinks the new worker just installed, and
if `sinksInitialized` is still false `drainRemaining()` does `queue.clear()`,
discarding the terminal event and any `FlushRequest` (`flush()` then waits out
its 60s timeout). The new lock only serializes two `shutdown()` calls.
`LineageHubConcurrentShutdownTest` never emits while a shutdown is in progress.
OpenLineage makes the overlap likely: `sink.shutdown()` drains HTTP for up to
30s on this thread, and the run can finish in that window.
**Suggestion:** Take `shutdownLock` in `ensureWorkerStarted()` so a
completion event cannot resurrect the hub mid-shutdown. Also gate
`LineageHub.shutdown()` on the hop-run hook: hold the gate across
`stopAndWait()` so `HopEnvironment`'s hook blocks until the terminal event is
queued, then drain once. Add a test that calls `shutdown()` on a slow sink
while another thread `emit`s, and assert a following `shutdown()` still
delivers that event.
##########
engine/src/main/java/org/apache/hop/run/HopRunBase.java:
##########
@@ -156,13 +158,24 @@ public abstract class HopRunBase implements Runnable,
IHasHopMetadataProvider {
+ "this flag is the CLI equivalent of the GUI \"Run anyway\"
override.")
protected boolean allowUnsupported = false;
+ @CommandLine.Option(
+ names = {"-swt", "--shutdown-timeout"},
+ description =
+ "The maximum number of seconds to wait for the pipeline or workflow
to stop when hop-run "
+ + "receives a termination signal (SIGTERM, SIGINT, SIGHUP), for
example from docker "
+ + "stop or Kubernetes. Hop Run stops the run like the Stop
button in Hop GUI does. "
+ + "Can also be set with the HOP_RUN_SHUTDOWN_TIMEOUT environment
variable.",
+ defaultValue = "${env:HOP_RUN_SHUTDOWN_TIMEOUT:-25}")
Review Comment:
**[suggestion]** The default wait is 25 seconds, but the new manual says
`docker stop` grants 10 seconds (`docker stop --time`) before SIGKILL. The
timeout is only a maximum, so a run that stops quickly still exits in time, but
anything slower is killed inside `stopAndWait()` before `afterStop` delivers
lineage. Kubernetes' default 30s grace does fit; Docker's does not, and the
docs also ask to leave room for the lineage drain.
**Suggestion:** Default to a value under 10 seconds (and still leave room
under the Kubernetes grace period for `LineageHub.shutdown()`), or set a
matching stop grace on the image and say so next to `HOP_RUN_SHUTDOWN_TIMEOUT`.
--
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]
