You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "urbandan (via GitHub)" <gi...@apache.org> on 2023/04/13 09:48:27 UTC
[GitHub] [kafka] urbandan opened a new pull request, #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
urbandan opened a new pull request, #13557:
URL: https://github.com/apache/kafka/pull/13557
…d thread to avoid stack overflows
KafkaStatusBackingStore uses an infinite retry logic on producer send, which can lead to a stack overflow. To avoid the problem, a background thread was added, and the callback submits the retry onto the background thread.
Change-Id: Ia2e4eb0a39e370df139a23b9c816ccaff14eb166
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] viktorsomogyi commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "viktorsomogyi (via GitHub)" <gi...@apache.org>.
viktorsomogyi commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1518581478
@vamossagar12 sure, I missed that. Will review it then.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] urbandan commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "urbandan (via GitHub)" <gi...@apache.org>.
urbandan commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1506674333
@viktorsomogyi Can you please review?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "vamossagar12 (via GitHub)" <gi...@apache.org>.
vamossagar12 commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1518570804
> @gharris1727 thanks for the heads up, I assigned the jira to myself, we'll take a look early next week.
Hey @viktorsomogyi actually me and @urbandan had already identified the flaky test in this pr https://github.com/apache/kafka/pull/13594 and the fix exists there . Can you plz review that pr ?
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] viktorsomogyi commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "viktorsomogyi (via GitHub)" <gi...@apache.org>.
viktorsomogyi commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1510892158
Seen 32 failures. Didn't look at them one by one because I think they are flaky but I restarted the run to see if we get a green one. Once they're complete (hopefully by the afternoon) I'll merge the PR.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] urbandan commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "urbandan (via GitHub)" <gi...@apache.org>.
urbandan commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1512514338
@viktorsomogyi I see 1 failure, but that doesn't seem related to the change.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] gharris1727 commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1518351859
Looks like this might have caused some flaky failures on trunk: https://issues.apache.org/jira/browse/KAFKA-14929
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "vamossagar12 (via GitHub)" <gi...@apache.org>.
vamossagar12 commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1510826990
Sure that makes sense. I will create a separate JIRA for this.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] viktorsomogyi merged pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "viktorsomogyi (via GitHub)" <gi...@apache.org>.
viktorsomogyi merged PR #13557:
URL: https://github.com/apache/kafka/pull/13557
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] viktorsomogyi commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "viktorsomogyi (via GitHub)" <gi...@apache.org>.
viktorsomogyi commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1518545419
@gharris1727 thanks for the heads up, I assigned the jira to myself, we'll take a look early next week.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] urbandan commented on a diff in pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "urbandan (via GitHub)" <gi...@apache.org>.
urbandan commented on code in PR #13557:
URL: https://github.com/apache/kafka/pull/13557#discussion_r1166859143
##########
clients/src/main/java/org/apache/kafka/common/utils/ThreadUtils.java:
##########
@@ -52,4 +55,26 @@ public Thread newThread(Runnable r) {
}
};
}
+
+ /**
+ * Shuts down an executor service with a timeout. After the timeout/on interrupt, the service is forcefully closed.
+ * @param executorService The service to shut down.
+ * @param timeout The timeout of the shutdown.
+ * @param timeUnit The time unit of the shutdown timeout.
+ * @return True, if the shutdown was clean.
+ */
+ public static boolean shutdownExecutorServiceSilently(ExecutorService executorService,
Review Comment:
thanks for the review, I fixed the method
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] urbandan commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "urbandan (via GitHub)" <gi...@apache.org>.
urbandan commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1512610083
@viktorsomogyi thank you!
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "vamossagar12 (via GitHub)" <gi...@apache.org>.
vamossagar12 commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1519560208
no problem @viktorsomogyi and thanks! Let me know if you would prefer a separate PR with just the flaky test fix.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] viktorsomogyi commented on a diff in pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "viktorsomogyi (via GitHub)" <gi...@apache.org>.
viktorsomogyi commented on code in PR #13557:
URL: https://github.com/apache/kafka/pull/13557#discussion_r1166798950
##########
clients/src/main/java/org/apache/kafka/common/utils/ThreadUtils.java:
##########
@@ -52,4 +55,26 @@ public Thread newThread(Runnable r) {
}
};
}
+
+ /**
+ * Shuts down an executor service with a timeout. After the timeout/on interrupt, the service is forcefully closed.
+ * @param executorService The service to shut down.
+ * @param timeout The timeout of the shutdown.
+ * @param timeUnit The time unit of the shutdown timeout.
+ * @return True, if the shutdown was clean.
+ */
+ public static boolean shutdownExecutorServiceSilently(ExecutorService executorService,
Review Comment:
Since the return value is never used and similar methods (like org.apache.kafka.common.utils.Utils#closeQuietly) are void, we can make this void too.
nit: please rename it to shutdownExecutorServiceQuietly to keep a similar naming to others.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] vamossagar12 commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "vamossagar12 (via GitHub)" <gi...@apache.org>.
vamossagar12 commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1509738740
Thanks for the PR @urbandan . This looks good. Now that you have introduced this method, I am thinking if other executors can use this method? I can see `herderExectuor`, `forwardRequestExecutor`, `startAndStopExecutor` all do a similar thing within `DistributedHerder`. We can do it in this PR or a follow up one if needed. LGTM otherwise 👍
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] viktorsomogyi commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "viktorsomogyi (via GitHub)" <gi...@apache.org>.
viktorsomogyi commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1512601412
Ok, I ran the test with failures locally and they seem to be fine, so flaky. The ForwardingAdmin tests are being fixed by #13575.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] urbandan commented on pull request #13557: KAFKA-14902: KafkaStatusBackingStore retries on a dedicated backgroun…
Posted by "urbandan (via GitHub)" <gi...@apache.org>.
urbandan commented on PR #13557:
URL: https://github.com/apache/kafka/pull/13557#issuecomment-1510809364
@vamossagar12 Thanks for the review. I agree, the util should be reused, but I would rather address that in a follow up PR.
--
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: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org