feat: Add multi-server test using the replicated queue manager. #380
Conversation
Summary of ChangesHello @kabir, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the event queue replication system, focusing on reliability and correctness in distributed and asynchronous scenarios. It addresses a critical bug that caused event loss in blocking API calls by ensuring continuous background consumption. A new transaction-aware "poison pill" mechanism guarantees that distributed queues are gracefully terminated only after task finalization is durably committed, preventing race conditions. The core event queue logic has been upgraded with an Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
ee415af to
80eedbe
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust and well-designed mechanism for event replication and queue management in a distributed environment. The implementation of a transaction-aware "poison pill" using CDI events is an excellent solution to prevent race conditions during task finalization. The refactoring from a ThreadLocal to an EventQueueItem interface to handle replication loops is a significant improvement in clarity and correctness. Furthermore, the addition of comprehensive multi-instance integration tests using Testcontainers provides high confidence in the new replication logic. The changes to the queue lifecycle, allowing queues for non-finalized tasks to persist, correctly supports fire-and-forget patterns and late resubscriptions. Overall, this is a very strong set of changes that greatly enhances the distributed capabilities of the system.
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive multi-server integration test for the replicated queue manager using Testcontainers. The setup with two Quarkus applications, a shared database, and Kafka is well-structured. The test logic correctly validates event replication between instances. I've added a few comments to improve code quality and maintainability, mainly regarding logging practices in the new test class and a design improvement for a utility class. Overall, this is a great addition for ensuring the robustness of the replication feature.
...s/queuemanager/replicated/tests/multiinstance/common/MultiInstanceReplicationAgentCards.java
Outdated
Show resolved
Hide resolved
.../io/a2a/extras/queuemanager/replicated/tests/multiinstance/MultiInstanceReplicationTest.java
Outdated
Show resolved
Hide resolved
| * - A2A Client instances to interact with both applications | ||
| */ | ||
| @Testcontainers | ||
| public class MultiInstanceReplicationTest { |
There was a problem hiding this comment.
This test class uses System.out.println, System.err.println, and e.printStackTrace() for logging. It is a better practice to use a dedicated logging framework like SLF4J, which is already a dependency in this module. Using a logger provides better control over log levels, formatting, and can be configured for different environments. It also ensures that exception stack traces are logged correctly.
You could introduce a logger like this:
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
// ...
public class MultiInstanceReplicationTest {
private static final Logger logger = LoggerFactory.getLogger(MultiInstanceReplicationTest.class);
// ...
}Then, you can replace calls like:
System.out.println("...")withlogger.info("...")orlogger.debug("...").System.err.println("...")withlogger.error("...").e.printStackTrace()withlogger.error("An error occurred", e);.
There was a problem hiding this comment.
I have removed most of the printlns, but I think for the error ones, they are good to keep to diagnose problems
…nd improve CI diagnostics - Filter HTTP/2 stream cancellation errors in KafkaReplicationIntegrationTest (same fix as PR a2aproject#380) - Add surefire reports and build logs upload to build-and-test workflow - Enhance TCK workflow to capture test output, server logs, and compliance reports Verified with 20/20 stress test passes.
…nd improve CI diagnostics - Filter HTTP/2 stream cancellation errors in KafkaReplicationIntegrationTest (same fix as PR a2aproject#380) - Add surefire reports and build logs upload to build-and-test workflow - Enhance TCK workflow to capture test output, server logs, and compliance reports
a2aproject#398) …nd improve CI diagnostics - Filter HTTP/2 stream cancellation errors in KafkaReplicationIntegrationTest (same fix as PR a2aproject#380) - Add surefire reports and build logs upload to build-and-test workflow - Enhance TCK workflow to capture test output, server logs, and compliance reports
No description provided.