-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[TH2-1988] separate connections for publishing and subscribing #318
Conversation
Test Results 31 files 31 suites 5s ⏱️ Results for commit 6b5c84a. ♻️ This comment has been updated with latest results. |
.../java/com/exactpro/th2/common/schema/message/impl/rabbitmq/connection/ConnectionManager.java
Outdated
Show resolved
Hide resolved
.../java/com/exactpro/th2/common/schema/message/impl/rabbitmq/connection/ConnectionManager.java
Outdated
Show resolved
Hide resolved
private final Map<PinId, ChannelHolder> channelsByPin = new ConcurrentHashMap<>(); | ||
private final AtomicReference<State> connectionIsClosed = new AtomicReference<>(State.OPEN); | ||
private final AtomicReference<State> connectionState = new AtomicReference<>(State.OPEN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this implementation state is shared between to real connection. Maybe instead of correct current Connection Manager, create ConnectionRouter witch delegates publish / subscribe requests to two instance of origin Connection manager.
src/main/kotlin/com/exactpro/th2/common/metrics/CommonMetrics.kt
Outdated
Show resolved
Hide resolved
...lin/com/exactpro/th2/common/schema/message/impl/rabbitmq/connection/TestConnectionManager.kt
Outdated
Show resolved
Hide resolved
assertEquals(blockAfter.toLong(), messagesCount - countDown.count) | ||
|
||
// unblocks publishers | ||
rabbit.executeInContainerWithLogging("rabbitmqctl", "set_vm_memory_high_watermark", "0.4") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we unlock the publisher in this test?
retryTimeDeviationPercent = 0 | ||
) | ||
).use { manager -> | ||
repeat(messagesCount) { index -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
messagesCount is greater than blockAfter, it means that publisher not blocking in this test because check logic is after this circle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We send 3 messages, then block publishers, then send 7 more. Subscribe, receive 3, unblock, receive 7. This allows to ensure that publishers were actually blocked. We can simplify the scenario: just send a few messages, block publishers, try to read messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then send 7 more
-> If publisher blocked we could able to publish additional messages
…t simplified getMonitorName fixed
…ic rollback isPublishingBlocked property Awaitility used
@Nikita-Smirnov-Exactpro @lumber1000 Should we close this PR after #321 is merged? |
No description provided.