uranusjr commented on code in PR #75538: URL: https://github.com/apache/airflow/pull/75538#discussion_r4534722249
########## java-sdk/sdk/src/main/kotlin/org/apache/airflow/sdk/Server.kt: ########## @@ -42,9 +45,39 @@ import org.apache.airflow.sdk.execution.comm.StartupDetails import org.apache.airflow.sdk.execution.parseDags import org.apache.airflow.sdk.execution.runTask import java.io.File +import java.time.Duration import kotlin.text.substringAfterLast import kotlin.text.substringBeforeLast +// The shutdown hook stops waiting for the queued log lines once none has gone out for this long. When the lines did +// go out, the hook also waits at most this long for the socket to close. The supervisor can stop reading the logs +// for several seconds, for example while it waits for a slow request to the API server. The hook should not lose the +// last lines during such a pause. Tests set a shorter timeout. +internal var logFlushTimeout: Duration = Duration.ofSeconds(30) Review Comment: This is only mutable so tests can shorten it. Can `sendQueuedLogs` take the timeout as a parameter instead so tests can pass another value? We’ll be able to remove this global entirely (a default on function arg would do) ########## java-sdk/sdk/src/main/kotlin/org/apache/airflow/sdk/execution/Logger.kt: ########## Review Comment: This PR adds more text than code to this file. I think most comments can be cut to one line or removed entirely. -- 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]
