Skip to content
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

JBTM-2982 Drive LRA participants from a separate thread #1332

Merged
merged 1 commit into from Jun 8, 2018

Conversation

Projects
None yet
3 participants
@mmusgrov
Copy link
Contributor

commented Jun 7, 2018

https://issues.jboss.org/browse/JBTM-2982

!TOMCAT !RTS !QA_JTA !QA_JTS_JDKORB !QA_JTS_OPENJDKORB !QA_JTS_JACORB !BLACKTIE !XTS !PERF !AS_TESTS !mysql !postgres !db2 !oracle

@jbosstm-bot

This comment has been minimized.

Copy link

commented Jun 7, 2018

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

Copy link

commented Jun 7, 2018

@jbosstm-bot

This comment has been minimized.

@mmusgrov mmusgrov requested a review from ochaloup Jun 7, 2018

@mmusgrov mmusgrov changed the title JBTM-2982 Drive participants from a separate thread JBTM-2982 Drive LRA participants from a separate thread Jun 7, 2018

@ochaloup
Copy link
Member

left a comment

I have a minor points about indentations, plus I have one doubt on the delaying of the end call. I don't understand the rationale.

On top of it on more minor point - I can see io.narayana.lra.coordinator.domain.model.LRARecord#tryDoEnd creating variable isRecovering which is used nowhere in the method. Should not be removed from the code?

private void updateStatus(boolean compensate) {
if (compensate)
status = accepted ? CompensatorStatus.Compensating : CompensatorStatus.Compensated;
else

This comment has been minimized.

Copy link
@ochaloup

ochaloup Jun 8, 2018

Member

maybe fixing indentation?

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Jun 8, 2018

Author Contributor

done

@@ -35,14 +35,14 @@
-Xrunjdwp:transport=dt_socket,address=${coordinator.debug.port},server=y,suspend=y
-->
<coordinator.debug.port>8787</coordinator.debug.port>
<coordinator.debug.jdwp>-Xrunjdwp:transport=dt_socket,address=${coordinator.debug.port},server=y,suspend=y</coordinator.debug.jdwp>
<coordinator.debug.jdwp>-Xrunjdwp:transport=dt_socket,address=${coordinator.debug.port},server=y,suspend=n</coordinator.debug.jdwp>
<coordinator.debug.params></coordinator.debug.params>

<!-- config for the test JAX-RS resource, for exampe to enable debugging use

This comment has been minimized.

Copy link
@ochaloup

ochaloup Jun 8, 2018

Member

do we need here this comment when the swarm.debug.jdwp attribute covers this?

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Jun 8, 2018

Author Contributor

What do you mean, do you want me to add an explanation of how the debugging config works?

This comment has been minimized.

Copy link
@ochaloup

ochaloup Jun 8, 2018

Member

sure. I'm sorry for not being clear. I just meant that if there is declared coordinator.debug.jdwp with debug parameters the comment could be removed completely. The information is doubled.

@@ -35,14 +35,14 @@
-Xrunjdwp:transport=dt_socket,address=${coordinator.debug.port},server=y,suspend=y

This comment has been minimized.

Copy link
@ochaloup

ochaloup Jun 8, 2018

Member

do we need here this comment when the swarm.debug.jdwp attribute covers this?

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Jun 8, 2018

Author Contributor

I can add more comments but it there are already comments and I thought those were clear.
There are two debug sections at play here, one for the coordinator and the other for the test resource. The two sections are separated by a blank line and each section includes a comment about which component the debug params apply to.

Can you indicate which bit needs more explanation.

This comment has been minimized.

Copy link
@ochaloup

ochaloup Jun 8, 2018

Member

I just meant if information is not doubled and if you consider so the comment could be removed competely.

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Jun 8, 2018

Author Contributor

okay

}

try {
Thread.sleep(ms <= 0 ? Long.MAX_VALUE : ms); // delay the end call

This comment has been minimized.

Copy link
@ochaloup

ochaloup Jun 8, 2018

Member

I would like to understand the rationalization of giving to user a way of delaying end call?
It's seems as some workaround to something. I can be a wrong in understanding but letting the client to drive timing from outside sounds to me supportive of race conditions.
I know we discussed some timeouts to be involved but I can't connect this with the discussion.

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Jun 8, 2018

Author Contributor

This is a test. The intent is to show that LRA can cope with participants that are slow in responding to the complete/compensate calls or, worse, participants that hang.

See the following tests that inject problems in participant end processing:

SpecIT#closeLRAWaitForRecovery
SpecIT#closeLRAWaitIndefinitely
SpecIT#connectionHangup

This comment has been minimized.

Copy link
@ochaloup

ochaloup Jun 8, 2018

Member

ouch, I see. Even I was checking the package I missed that. I'm sorry, this is fine.

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Jun 8, 2018

Author Contributor

If I push a commit for the indentation and remove the jdwp debug comment will that fix your issues with this PR.

This comment has been minimized.

Copy link
@mmusgrov

mmusgrov Jun 8, 2018

Author Contributor

Notice that I also added a comment to indicate what the recovery scan is testing here.

@mmusgrov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2018

@ochaloup You said the variable isRecovering in LRARecord.java isn't used but it is used on line 299

@ochaloup

This comment has been minimized.

Copy link
Member

commented Jun 8, 2018

@mmusgrov hm... I have probably different version, I can't find the isRecovering at more places in the LRARecord. If I take what is here in the PR: https://github.com/mmusgrov/narayana/blob/43e5c9c9a93cf58db1b25b8a1a86b76e27622bd2/rts/lra/lra-coordinator/src/main/java/io/narayana/lra/coordinator/domain/model/LRARecord.java#L242 then the variable is declared at the l242 and then I can't find any usage of it. What is your version of the code?

@mmusgrov mmusgrov force-pushed the mmusgrov:JBTM-2982 branch from 43e5c9c to f5268fa Jun 8, 2018

@mmusgrov

This comment has been minimized.

Copy link
Contributor Author

commented Jun 8, 2018

@ochaloup re isRecovering, it was me that was looking at a wrong version, sorry.

So I have pushed a commit with that, the extra tab and the jdwp debug comment removed. None of those are code changes so if you are satisfied then I don't think we need to wait for CI?

@jbosstm-bot

This comment has been minimized.

Copy link

commented Jun 8, 2018

@jbosstm-bot

This comment has been minimized.

@jbosstm-bot

This comment has been minimized.

Copy link

commented Jun 8, 2018

@ochaloup
Copy link
Member

left a comment

thanks @mmusgrov , no more ideas from my side 👍 😄

@mmusgrov mmusgrov merged commit 49b6205 into jbosstm:master Jun 8, 2018

@mmusgrov mmusgrov deleted the mmusgrov:JBTM-2982 branch Jun 8, 2018

@jbosstm-bot

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.