[TH2-2212] merged with dev 5 #283
Conversation
…n, added logging for hard errors
…' of github.com:th2-net/th2-common-j into TH2-2212-common-cannot-recover-channel-level-exceptions
|
@Nikita-Smirnov-Exactpro Could you please check the PR and if you don't have any objections I think we can merge it |
| @@ -189,6 +189,10 @@ private SubscriberMonitor basicConsume() { | |||
| return connectionManager.basicConsume(queue, this::handle, this::canceled); | |||
| } catch (IOException e) { | |||
| throw new IllegalStateException("Can not subscribe to queue = " + queue, e); | |||
| } catch (InterruptedException e) { | |||
There was a problem hiding this comment.
Why do we catch InterruptedException in here (private method). I think InterruptedException should be a signal for interruption of retry or subscribe process
There was a problem hiding this comment.
If we are in catch block subscribe retry process is already interrupted (because it's in try block - connectionManager.basicConsume).
.../com/exactpro/th2/common/schema/message/impl/rabbitmq/configuration/RabbitMQConfiguration.kt
Show resolved
Hide resolved
.../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
.../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
Show resolved
Hide resolved
.../java/com/exactpro/th2/common/schema/message/impl/rabbitmq/connection/ConnectionManager.java
Outdated
Show resolved
Hide resolved
filtering out recovering tasks for non subscribed channels
...lin/com/exactpro/th2/common/schema/message/impl/rabbitmq/connection/TestConnectionManager.kt
Outdated
Show resolved
Hide resolved
| } finally { | ||
| lock.unlock(); | ||
|
|
||
| TimeUnit.MILLISECONDS.sleep(recoveryDelay); |
There was a problem hiding this comment.
Why this instruction was moved from catch block?
There was a problem hiding this comment.
To avoid locking for a long time.
There was a problem hiding this comment.
all operation with a channel are executed under lock. If one operation faces to a retry problem another will wait too.
In this case we can block another operations until resolve the retry problem
There was a problem hiding this comment.
ok, i will add one more lock for processing callbacks.
There was a problem hiding this comment.
No, please satay as is
There was a problem hiding this comment.
Several publishers follow replay schedule in your case.
There was a problem hiding this comment.
we need 2nd lock to read channel and isSubscribed values to filter out unnecessary callbacks without waiting for channel recovery. Otherwise all callbacks will be accumulated in executor service queue until the connection and channels will be recovered (and every recovery attempt generates new callback).
There was a problem hiding this comment.
We can use volatile or AtomicBoolean instead. Less lock, simple code, less chance to make a deadlock in future changes
dependencies update
.../java/com/exactpro/th2/common/schema/message/impl/rabbitmq/connection/ConnectionManager.java
Outdated
Show resolved
Hide resolved
|
@Nikita-Smirnov-Exactpro @lumber1000 Hi, what are we doing about this one? If we have tested it let's pull the latest change and if integration tests pass merge the PR |
I agree with you, because we haven't got other tests |
# Conflicts: # README.md # gradle.properties
version updated
No description provided.