You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "brandboat (via GitHub)" <gi...@apache.org> on 2024/08/04 09:12:22 UTC
[PR] KAFKA-17235 system test test_performance_service.py failed [kafka]
brandboat opened a new pull request, #16789:
URL: https://github.com/apache/kafka/pull/16789
related to https://issues.apache.org/jira/browse/KAFKA-17235
The root cause of this issue is a change we introduced in [KAFKA-16879](https://issues.apache.org/jira/browse/KAFKA-16879), where we modified the PushHttpMetricsReporter constructor to use Time.System [1]. However, Time.System doesn't exist in Kafka versions 0.8.2 and 0.9.
In test_performance_services.py, we have system tests for Kafka versions 0.8.2 and 0.9 [2]. These tests always use the tools JAR from the trunk branch, regardless of the Kafka version being tested [3], while the client JAR aligns with the Kafka version specified in the test suite [4]. This discrepancy is what causes the issue to arise.
To resolve this issue, we have a few options:
- Add Time.System to Kafka 0.8.2 and 0.9: This isn't practical, as we no longer maintain these versions.
- Modify the PushHttpMetricsReporter constructor to use new SystemTime() instead of Time.System: This would contradict the intent of [KAFKA-16879](https://issues.apache.org/jira/browse/KAFKA-16879), which aims to make SystemTime a singleton.
- Remove system tests for Kafka 0.8.2 and 0.9 from test_performance_services.py
Given that we no longer maintain Kafka 0.8.2 and 0.9, and altering the constructor goes against the design goals of [KAFKA-16879](https://issues.apache.org/jira/browse/KAFKA-16879), option 3 appears to be the most feasible solution. However, I'm not sure whether it's acceptable to remove these old version tests. Maybe someone else has a better solution
[1] https://github.com/apache/kafka/blob/133f2b0f311ba1fd5a999f477bad38370c1772ca/tools/src/main/java/org/apache/kafka/tools/PushHttpMetricsReporter.java#L99
[2] https://github.com/apache/kafka/blob/0711d1558250c60807dfa815d46907eac8bb4b98/tests/kafkatest/sanity_checks/test_performance_services.py#L43-L44
[3] https://github.com/apache/kafka/blob/0711d1558250c60807dfa815d46907eac8bb4b98/tests/kafkatest/services/performance/producer_performance.py#L89
[4] https://github.com/apache/kafka/blob/0711d1558250c60807dfa815d46907eac8bb4b98/tests/kafkatest/services/performance/producer_performance.py#L80
### 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
Re: [PR] KAFKA-17235 system test test_performance_service.py failed [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #16789:
URL: https://github.com/apache/kafka/pull/16789#discussion_r1704028683
##########
tools/src/main/java/org/apache/kafka/tools/PushHttpMetricsReporter.java:
##########
@@ -96,7 +97,31 @@ public class PushHttpMetricsReporter implements MetricsReporter {
"producer/consumer/streams/connect instance");
public PushHttpMetricsReporter() {
- time = Time.SYSTEM;
+ // This is for backward compatibility with older versions like 0.8 and 0.9,
Review Comment:
Could you please includes following items in docs:
1. this is used by xxx E2E
2. the E2E run this report with older client jars
3. hence this class must be compatible with older client jars.
--
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
Re: [PR] KAFKA-17235 system test test_performance_service.py failed [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on PR #16789:
URL: https://github.com/apache/kafka/pull/16789#issuecomment-2269297940
> @brandboat Could you please verify the e2e on your local?
Yes, I run cmd as follows
```shell
./gradlew clean build -x test
docker rm -f ducker01 ducker02 ducker03 ducker04 ducker05 ducker06 ducker07 ducker08 ducker09 ducker10 ducker11 ducker12 ducker13 ducker14
TC_PATHS="tests/kafkatest/sanity_checks/test_performance_services.py" bash tests/docker/run_tests.sh
```
and test passed
```shell
================================================================================
SESSION REPORT (ALL TESTS)
ducktape version: 0.11.4
session_id: 2024-08-05--002
run time: 3 minutes 13.730 seconds
tests run: 7
passed: 7
flaky: 0
failed: 0
ignored: 0
================================================================================
```
--
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
Re: [PR] KAFKA-17235 system test test_performance_service.py failed [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16789:
URL: https://github.com/apache/kafka/pull/16789#issuecomment-2269277672
@brandboat Could you please verify the e2e on your local?
--
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
Re: [PR] KAFKA-17235 system test test_performance_service.py failed [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on code in PR #16789:
URL: https://github.com/apache/kafka/pull/16789#discussion_r1704119775
##########
tools/src/main/java/org/apache/kafka/tools/PushHttpMetricsReporter.java:
##########
@@ -96,7 +97,31 @@ public class PushHttpMetricsReporter implements MetricsReporter {
"producer/consumer/streams/connect instance");
public PushHttpMetricsReporter() {
- time = Time.SYSTEM;
+ // This is for backward compatibility with older versions like 0.8 and 0.9,
Review Comment:
> another fix is that we can change time from Time to Supplier<Long> to offer current time. WDYT?
Thanks for the review, this one looks better.
--
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
Re: [PR] KAFKA-17235 system test test_performance_service.py failed [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #16789:
URL: https://github.com/apache/kafka/pull/16789#issuecomment-2267622823
@brandboat nice finding!
The total supports of 0.8 and 0.9 are not dropped. At least, we expect the producer should work. Hence, I prefer to solution 2 - make `PushHttpMetricsReporter` work with 0.8
`PushHttpMetricsReporter` use the `time` to get current time, and hence we can do a bit refactor for it. For example: `PushHttpMetricsReporter` can implement the `Time` to offer the current time.
--
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
Re: [PR] KAFKA-17235 system test test_performance_service.py failed [kafka]
Posted by "brandboat (via GitHub)" <gi...@apache.org>.
brandboat commented on PR #16789:
URL: https://github.com/apache/kafka/pull/16789#issuecomment-2268139439
> The total supports of 0.8 and 0.9 are not dropped. At least, we expect the producer should work. Hence, I prefer to solution 2 - make PushHttpMetricsReporter work with 0.8
>
> PushHttpMetricsReporter use the time to get current time, and hence we can do a bit refactor for it. For example: PushHttpMetricsReporter can implement the Time to offer the current time.
Thanks for the suggestion, I've addressed everything you mentioned, could you take another look ?
--
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
Re: [PR] KAFKA-17235 system test test_performance_service.py failed [kafka]
Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #16789:
URL: https://github.com/apache/kafka/pull/16789
--
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