You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "gharris1727 (via GitHub)" <gi...@apache.org> on 2023/02/22 20:32:02 UTC
[GitHub] [kafka] gharris1727 opened a new pull request, #13291: KAFKA-14742: Throttle connectors in ExactlyOnceSourceIntegrationTest to fix flakey OOMEs
gharris1727 opened a new pull request, #13291:
URL: https://github.com/apache/kafka/pull/13291
On my local machine, testIntervalBoundary is asserting on nearly 2.5 million records, when it appears that the test is written to need only 100-1000 records to perform assertions. This causes OOMEs in the test assertions which iterate over the set of records and perform memory allocations.
I looked into reducing the assertion's memory overhead, but it didn't seem practical as even the smallest allocations appeared to exceed the memory limit.
Instead, I configured the pre-existing throttle mechanism inside the MonitorableSourceConnector, so that tests now seem to produce ~90k records on my machine, leaving adequate spare memory for the existing assertions to pass without issue.
### 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] gharris1727 commented on a diff in pull request #13291: KAFKA-14742: Throttle connectors in ExactlyOnceSourceIntegrationTest to fix flakey OOMEs
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #13291:
URL: https://github.com/apache/kafka/pull/13291#discussion_r1117488058
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExactlyOnceSourceIntegrationTest.java:
##########
@@ -266,6 +267,7 @@ public void testPollBoundary() throws Exception {
props.put(NAME_CONFIG, CONNECTOR_NAME);
props.put(TRANSACTION_BOUNDARY_CONFIG, POLL.toString());
props.put(MESSAGES_PER_POLL_CONFIG, Integer.toString(recordsProduced));
+ props.put(THROUGHPUT_CONFIG, Integer.toString(recordsProduced));
Review Comment:
Since the result of Integer.toString(100) and Long.toString(100L) are the same, I don't think this necessary.
The reason I re-used the same variable was because I wanted to keep the runtime of the test constant. If there were two variables, someone could tune one while holding the other constant until the test timed out.
I agree that `recordsProduced` is a poor name, because this test produces many more records than that under normal conditions. Do you have a better name in mind?
--
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] edoardocomar commented on a diff in pull request #13291: KAFKA-14742: Throttle connectors in ExactlyOnceSourceIntegrationTest to fix flakey OOMEs
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on code in PR #13291:
URL: https://github.com/apache/kafka/pull/13291#discussion_r1118563985
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExactlyOnceSourceIntegrationTest.java:
##########
@@ -81,6 +81,7 @@
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.CUSTOM_EXACTLY_ONCE_SUPPORT_CONFIG;
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.CUSTOM_TRANSACTION_BOUNDARIES_CONFIG;
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.MESSAGES_PER_POLL_CONFIG;
+import static org.apache.kafka.connect.integration.MonitorableSourceConnector.THROUGHPUT_CONFIG;
Review Comment:
I had to go and look what unit `throughput` was in and `ThroughputThrottler` says `Can be messages/sec or bytes/sec` . In the case of this test it is messages, so for me teh longer name I suggested helps readability.
--
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] edoardocomar commented on a diff in pull request #13291: KAFKA-14742: Throttle connectors in ExactlyOnceSourceIntegrationTest to fix flakey OOMEs
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on code in PR #13291:
URL: https://github.com/apache/kafka/pull/13291#discussion_r1117306285
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExactlyOnceSourceIntegrationTest.java:
##########
@@ -266,6 +267,7 @@ public void testPollBoundary() throws Exception {
props.put(NAME_CONFIG, CONNECTOR_NAME);
props.put(TRANSACTION_BOUNDARY_CONFIG, POLL.toString());
props.put(MESSAGES_PER_POLL_CONFIG, Integer.toString(recordsProduced));
+ props.put(THROUGHPUT_CONFIG, Integer.toString(recordsProduced));
Review Comment:
the config is a Long, so these settings could be
Long.toString(100L)
I checked the test that OOM'ing for me too and the number of records actually produced with your setting is still much larger than actually erquired.
I found using the same variable `recordsProduced` for throughput was a bit puzzling, maybe just using another literal would be ok.
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExactlyOnceSourceIntegrationTest.java:
##########
@@ -81,6 +81,7 @@
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.CUSTOM_EXACTLY_ONCE_SUPPORT_CONFIG;
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.CUSTOM_TRANSACTION_BOUNDARIES_CONFIG;
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.MESSAGES_PER_POLL_CONFIG;
+import static org.apache.kafka.connect.integration.MonitorableSourceConnector.THROUGHPUT_CONFIG;
Review Comment:
this could be THROUGHPUT_MSGS_PER_SEC_CONFIG
--
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] edoardocomar commented on a diff in pull request #13291: KAFKA-14742: Throttle connectors in ExactlyOnceSourceIntegrationTest to fix flakey OOMEs
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on code in PR #13291:
URL: https://github.com/apache/kafka/pull/13291#discussion_r1117309870
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExactlyOnceSourceIntegrationTest.java:
##########
@@ -266,6 +267,7 @@ public void testPollBoundary() throws Exception {
props.put(NAME_CONFIG, CONNECTOR_NAME);
props.put(TRANSACTION_BOUNDARY_CONFIG, POLL.toString());
props.put(MESSAGES_PER_POLL_CONFIG, Integer.toString(recordsProduced));
+ props.put(THROUGHPUT_CONFIG, Integer.toString(recordsProduced));
Review Comment:
even 50 msgs/sec will be enough
--
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 #13291: KAFKA-14742: Throttle connectors in ExactlyOnceSourceIntegrationTest to fix flakey OOMEs
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on PR #13291:
URL: https://github.com/apache/kafka/pull/13291#issuecomment-1447059390
@edoardocomar
I pulled these values out into three separate constants with descriptive names, let me know if this is closer to what you had in mind!
--
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] edoardocomar commented on a diff in pull request #13291: KAFKA-14742: Throttle connectors in ExactlyOnceSourceIntegrationTest to fix flakey OOMEs
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on code in PR #13291:
URL: https://github.com/apache/kafka/pull/13291#discussion_r1118569732
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExactlyOnceSourceIntegrationTest.java:
##########
@@ -266,6 +267,7 @@ public void testPollBoundary() throws Exception {
props.put(NAME_CONFIG, CONNECTOR_NAME);
props.put(TRANSACTION_BOUNDARY_CONFIG, POLL.toString());
props.put(MESSAGES_PER_POLL_CONFIG, Integer.toString(recordsProduced));
+ props.put(THROUGHPUT_CONFIG, Integer.toString(recordsProduced));
Review Comment:
Again, it may not be *necessary* to use Long instead of Integer but it helps.
And using two variables instead of one, although with related values is again helping readability.
Reusing the same one is something that makes me stop and think "why...?"
So having a 2nd variable e.g. like
`long throughput_msgs_sec = recordsProduced / 2L;`
would be my preference (and a short line comment for it) e.g.
// need to limit actual records.count() to avoid OOM
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExactlyOnceSourceIntegrationTest.java:
##########
@@ -266,6 +267,7 @@ public void testPollBoundary() throws Exception {
props.put(NAME_CONFIG, CONNECTOR_NAME);
props.put(TRANSACTION_BOUNDARY_CONFIG, POLL.toString());
props.put(MESSAGES_PER_POLL_CONFIG, Integer.toString(recordsProduced));
+ props.put(THROUGHPUT_CONFIG, Integer.toString(recordsProduced));
Review Comment:
Again, it may not be *necessary* to use Long instead of Integer but it helps.
And using two variables instead of one, although with related values is again helping readability.
Reusing the same one is something that makes me stop and think "why...?"
So having a 2nd variable e.g. like
`long throughput_msgs_sec = recordsProduced / 2L;`
would be my preference (and a short line comment for it) e.g.
`// need to limit actual records.count() to avoid OOM`
--
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 a diff in pull request #13291: KAFKA-14742: Throttle connectors in ExactlyOnceSourceIntegrationTest to fix flakey OOMEs
Posted by "gharris1727 (via GitHub)" <gi...@apache.org>.
gharris1727 commented on code in PR #13291:
URL: https://github.com/apache/kafka/pull/13291#discussion_r1117479545
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExactlyOnceSourceIntegrationTest.java:
##########
@@ -81,6 +81,7 @@
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.CUSTOM_EXACTLY_ONCE_SUPPORT_CONFIG;
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.CUSTOM_TRANSACTION_BOUNDARIES_CONFIG;
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.MESSAGES_PER_POLL_CONFIG;
+import static org.apache.kafka.connect.integration.MonitorableSourceConnector.THROUGHPUT_CONFIG;
Review Comment:
I think that would make sense if the underlying configuration was `throughput.msgs.per.sec` but it is currently `throughput`. I preferred to keep the existing name instead of renaming + aliasing the configuration. just to keep this PR small.
Do you think renaming the configuration is important here?
--
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] edoardocomar merged pull request #13291: KAFKA-14742: Throttle connectors in ExactlyOnceSourceIntegrationTest to fix flakey OOMEs
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar merged PR #13291:
URL: https://github.com/apache/kafka/pull/13291
--
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] edoardocomar commented on a diff in pull request #13291: KAFKA-14742: Throttle connectors in ExactlyOnceSourceIntegrationTest to fix flakey OOMEs
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on code in PR #13291:
URL: https://github.com/apache/kafka/pull/13291#discussion_r1118563985
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExactlyOnceSourceIntegrationTest.java:
##########
@@ -81,6 +81,7 @@
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.CUSTOM_EXACTLY_ONCE_SUPPORT_CONFIG;
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.CUSTOM_TRANSACTION_BOUNDARIES_CONFIG;
import static org.apache.kafka.connect.integration.MonitorableSourceConnector.MESSAGES_PER_POLL_CONFIG;
+import static org.apache.kafka.connect.integration.MonitorableSourceConnector.THROUGHPUT_CONFIG;
Review Comment:
I had to go and look what unit `throughput` was in and `ThroughputThrottler` says `Can be messages/sec or bytes/sec` that's why its name is generic.
In the case of this test, it is messages/sec, so for me the longer name I suggested helps readability.
--
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] edoardocomar commented on a diff in pull request #13291: KAFKA-14742: Throttle connectors in ExactlyOnceSourceIntegrationTest to fix flakey OOMEs
Posted by "edoardocomar (via GitHub)" <gi...@apache.org>.
edoardocomar commented on code in PR #13291:
URL: https://github.com/apache/kafka/pull/13291#discussion_r1118569732
##########
connect/runtime/src/test/java/org/apache/kafka/connect/integration/ExactlyOnceSourceIntegrationTest.java:
##########
@@ -266,6 +267,7 @@ public void testPollBoundary() throws Exception {
props.put(NAME_CONFIG, CONNECTOR_NAME);
props.put(TRANSACTION_BOUNDARY_CONFIG, POLL.toString());
props.put(MESSAGES_PER_POLL_CONFIG, Integer.toString(recordsProduced));
+ props.put(THROUGHPUT_CONFIG, Integer.toString(recordsProduced));
Review Comment:
Again, it may not be *necessary* to use Long instead of Integer but it helps. Property is a long, I'd prefer to set a long rather than rely on conversion.
And using two variables instead of one, although with related values is again helping readability.
Reusing the same one is something that makes me stop and think "why...?"
So having a 2nd variable e.g. like
`long throughput_msgs_sec = recordsProduced / 2L;`
would be my preference (and a short line comment for it) e.g.
`// need to limit actual records.count() to avoid OOM`
--
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