jerryshao commented on code in PR #14230:
URL: https://github.com/apache/gravitino/pull/14230#discussion_r4375496485
##########
plugins/k8s-job-executor/src/main/java/org/apache/gravitino/job/k8s/K8sJobExecutor.java:
##########
@@ -129,17 +139,13 @@ void initialize(K8sJobExecutorConfigs configs,
KubernetesClient client) {
this.sparkApplicationCache =
new NamespacedResourceCache<>(
configs.statusCacheTtlMs(),
- namespace ->
- getSparkApplicationClient(namespace)
- .withLabel(
- K8sJobResourceUtils.LABEL_MANAGED_BY,
- K8sJobResourceUtils.MANAGED_BY_GRAVITINO)
- .list()
- .getItems(),
- // Getting the SparkApplication itself couldn't reach the API
server either.
+ this::listSparkApplications,
+ // Getting the SparkApplication itself couldn't reach the API
server either. A listing
+ // that the API server is too slow to answer isn't, it gets the
jobs one by one instead.
e ->
e instanceof KubernetesClientException
- && isUnreachable((KubernetesClientException) e),
+ && isUnreachable((KubernetesClientException) e)
+ && !isResponseTimedOut((KubernetesClientException) e),
Review Comment:
[Question] The fallback depends on `HttpTimeoutException` still being the
cause of the `KubernetesClientException` that fabric8 hands the cache. The one
end-to-end test of it, `testSlowListingFallsBackToGet`, builds its client with
`clientWithoutRetries(...withRequestTimeout(200))`, i.e.
`requestRetryBackoffLimit = 0`, while the client the executor really uses sets
`requestRetries` (default 2) and `requestTimeoutMs` (default 10000) —
K8sClientUtils.java:171-172, K8sJobExecutorConfigs.java:82,94. Did you confirm
against a real cluster that after the retries are exhausted the exception still
carries `HttpTimeoutException` (and not, say, a wrapper), so a slow LIST is
classified as non-fatal in production too?
A second part of the same question: with those defaults a timing-out listing
takes roughly 3 x 10 s before it fails, and it is retried once per cache TTL
(default 5 s, `DEFAULT_STATUS_CACHE_TTL_MS`), so a poll over a namespace whose
listing times out pays ~30 s per TTL window even with this fix. Is that
acceptable, or is a shorter timeout (or a longer cached-failure TTL) worth it
for the listing specifically?
Verified by: read `isResponseTimedOut` (K8sJobExecutor.java:382-395) and the
`isFatal` lambda here, then the test at TestK8sJobExecutor.java
(`testSlowListingFallsBackToGet` and the `clientWithoutRetries` helper) and the
production client configuration in K8sClientUtils/K8sJobExecutorConfigs. Not a
defect I can demonstrate - the fabric8 jars are not in this sandbox, so I could
not read the retry code.
##########
core/src/main/java/org/apache/gravitino/connector/job/JobExecutor.java:
##########
@@ -189,9 +189,11 @@ default boolean isJobStateNodeLocal() {
* @param jobId The unique identifier of the job.
* @param maxLines The maximum number of (most recent) lines to return,
resolved by the caller
* from the {@code gravitino.job.outputMaxLines} configuration.
- * @param maxBytes The maximum number of (most recent) bytes to read from
the underlying output,
- * resolved by the caller from the {@code gravitino.job.outputMaxBytes}
configuration. Bounds
- * both the read cost and the response size regardless of how the
content is shaped.
+ * @param maxBytes The maximum number of (most recent) bytes to return,
resolved by the caller
+ * from the {@code gravitino.job.outputMaxBytes} configuration. Bounds
the response size
+ * regardless of how the content is shaped. An implementation should
also bound the bytes it
+ * reads from the underlying output when it can, but may not, for
example when the output can
+ * only be read by lines.
Review Comment:
[Nit] The contract is corrected here and in `Configs`, but two places in
core still carry the old wording: `JobOperationDispatcher#getJob` says "@param
maxBytes the maximum number of (most recent) output bytes to read"
(core/src/main/java/org/apache/gravitino/job/JobOperationDispatcher.java:141),
and `JobManager#getJob` comments the clamp with "the global configuration
remains a hard upper bound on read cost/response size"
(core/src/main/java/org/apache/gravitino/job/JobManager.java:494).
`JobOperationDispatcher` is the interface the REST layer calls, so its Javadoc
is the one a connector author reads next to this one. Suggest changing "to
read" to "to return" there and dropping "read cost" from the comment.
Verified by: grepped the repository for the old wording (`bytes to read`,
`read from the tail`, `read cost`) after reading this hunk; those two and
`PodLogUtils#tail` are the only hits, and `PodLogUtils` really does bound the
bytes it keeps, so its wording is fine.
--
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]
