[GitHub] [kafka] fvaleri commented on pull request #14092: KAFKA-15239: Fix system tests using producer performance service
fvaleri commented on PR #14092: URL: https://github.com/apache/kafka/pull/14092#issuecomment-1666442787 > That does appear to work, because [server-common is not being excluded from tools-dependant-libs](https://github.com/apache/kafka/blob/b3db905b27ff4133f4018ac922c9ce2beb2d6087/build.gradle#L1895-L1897) like the clients is. Exactly. > If we choose not to change this compatibility boundary from between server-common + clients to between tools + server-common, then I think the fix you propose is possible. I think we can improve further as you say, but probably it's better to do it in a separate PR, starting from working code. > Can you revert the changes in producer_performance.py? they don't seem to have an effect anymore. Done. Thanks again for the help, appreciated. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] fvaleri commented on pull request #14092: KAFKA-15239: Fix system tests using producer performance service
fvaleri commented on PR #14092: URL: https://github.com/apache/kafka/pull/14092#issuecomment-1665804677 @gharris1727 I tried various things, as I'm also learning how this works. The best solution I found is to move the ThroughputThrottler class from clients to server-common, adding this extra implementation dependency to connect:runtime (last commit on this branch). Why server-common? AFAIU, server-common hosts all classes shared by different modules, tools and connect:runtime in this case. Unlike tools, I think it makes sense to have this dependency for connect:runtime, as it may be used for other classes in the future. That way, we won't move the compatibility boundary. System tests will continue to use ProducerPerformance from dev branch as before, without bringing in dev clients, but using the version under test. Let me know if you see any issue with this approach. Thanks. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] fvaleri commented on pull request #14092: KAFKA-15239: Fix system tests using producer performance service
fvaleri commented on PR #14092: URL: https://github.com/apache/kafka/pull/14092#issuecomment-1665393332 > So for example, if the node.version is 1.0.0, this condition will be true, and the script will add DEV jars to CLASSPATH. kafka-run-class.sh then finds the 1.0.0 jars and adds them to CLASSPATH. This shows my lack of knowledge on Kafka STs. I see it now. Thanks. > In https://github.com/apache/kafka/pull/13313 I explored eliminating the artifact mixing, but it turned out that the 0.8.x tools was missing functionality, and so I had to inject the 0.9.x tools jar. I think we can do something similar here, try to remove the artifact mixing, and if that fails, find an intermediate version that can be injected instead of trunk. Yeah, I'll try to do that and give an update. -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] fvaleri commented on pull request #14092: KAFKA-15239: Fix system tests using producer performance service
fvaleri commented on PR #14092: URL: https://github.com/apache/kafka/pull/14092#issuecomment-1664186438 Hi @gharris1727, thanks for the clarification and sorry for the delay, I've been busy with other stuff. > By "version mixing" I mean that artifacts from two different versions from different versions are present on the classpath at the same time. I see, but that's not the case here, we either LATEST_0_9 or DEV_BRANCH. We do not load different module versions at the same time on the classpath, so there is no compatibility boundary to consider. Am I missing something? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [kafka] fvaleri commented on pull request #14092: KAFKA-15239: Fix system tests using producer performance service
fvaleri commented on PR #14092: URL: https://github.com/apache/kafka/pull/14092#issuecomment-1652091101 > if we loaded a 1.0.0 server-common class that depended on a class which was missing from DEV clients, I would expect that to fail. These changes are just for the ProducerPerformanceService, and we only need clients on dev release because ThroughputThrottler moved there. That said, I don't like this approach and I see the risk you mention, so I'm opened to proposals. > If possible, I think we should eliminate this long-range version mixing and compatibility boundaries, rather than move them around, because they're already seem to be in the least-impactful place and are still causing problems. Can you explain better what you mean 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
