[AMBARI-25325] : RetryUpgradeActionService not updating host_role_command in Transaction

[virajjasani]

What changes were proposed in this pull request?

non-public method in the same instance with guice @Transactional doesn't initiate Transaction at the beginning of the method and tear down after execution of the method. The same is the case with RetryUpgradeActionService updating host_role_command entries during Rolling Upgrade for commands with HOLDING_* failed status. Created a new Singleton class to offload the responsibility of creating Transaction using AOP proxy so that Transaction is applied to retryHoldingCommandsInRequest()

How was this patch tested?

The patch was tested manually during Rolling Upgrade as well as with unit testing available in RetryUpgradeActionServiceTest. Verified Transactional behavior with/without some Exception to Rollback/Commit the ongoing Transaction for both - before/after applying the patch

[asfgit]

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/5346/
Test FAILed.
Test FAILured.

[asfgit]

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/5347/
Test FAILed.
Test FAILured.

[asfgit]

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/5349/
Test PASSed.

[virajjasani]

@adoroszlai @zeroflag @jonathan-hurley Please review

[jonathan-hurley]

Why break this out into its own file? It's not going to be reused. Can you just make this method public instead?

[virajjasani]

@jonathan-hurley Thanks for your review.
Since guice Transactional annotation works through AOP proxy, the following holds true:
In proxy mode (which is the default), only external method calls coming in through the proxy are intercepted. This means that self-invocation, in effect, a method within the target object calling another method of the target object, will not lead to an actual transaction at runtime even if the invoked method is marked with @transactional. Only invocation to the method of another target object will create actual transaction.

[virajjasani]

If object A calls public transactional method of it's own, there would not be any proxy involved and hence, purpose of @transactional would not provide any meaning. Only if object A calls public transactional method of object B, given that object B is already injected to object A with proxy, @transactional creates actual transaction and commit/rollback the transaction based on the ongoing execution.

[asfgit]

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/5352/
Test PASSed.

[jonathan-hurley]

According to the @Transactional documentation, the only restriction is that the methods are public. We have objects invoking methods on themselves which begin transactions in many places (for example, in the ResourceProviders).

Where are you getting your information from?

[virajjasani]

@jonathan-hurley here is one of the links:
https://stackoverflow.com/questions/14096529/guice-transactional-does-not-start-an-transaction

Also, I tried to verify this scenario using unit tests available in RetryUpgradeActionServiceTest.java
Without applying this patch, tried to cause some Exception after dao.merge:

ambari/ambari-server/src/main/java/org/apache/ambari/server/state/services/RetryUpgradeActionService.java

Line 304 in c6db11c

m_hostRoleCommandDAO.merge(hrc);

(we can put may be: int i=1/0; after L304)

Scenario 1: Without this patch, with and without public method retryHoldingCommandsInRequest, dao.merge is not getting rolled back, which means Transaction is not applied to retryHoldingCommandsInRequest.

Scenario 2: With this patch, dao.merge is getting rolled back.

The only reason why this update was working so far is because m_hostRoleCommandDAO.merge(hrc); itself is annotated with Transactional and the dependency of m_hostRoleCommandDAO is injected to RetryUpgradeActionService. However, there is no Transaction getting applied for retryHoldingCommandsInRequest as such.

[virajjasani]

@jonathan-hurley There is one more way to further validate transaction applicable for retryHoldingCommandsInRequest() method:

Case 1: Without applying this patch, we can mark retryHoldingCommandsInRequest() as public here . Then we can comment out(or remove) m_hostRoleCommandDAO.merge(hrc); here . Now, if we run test, it would fail, which should not if Transaction was applied to retryHoldingCommandsInRequest() because any changes made to HostRoleCommandEntity within Transaction would get auto-committed to DB if the actual transaction was applied.

Case 2: After applying this patch, we can comment out(or remove) m_hostRoleCommandDAO.merge(hrc); here . Now if we run test, it would succeed because we don't need to explicitly merge(commit) HostRoleCommandEntity as any change within transaction would be auto-committed to DB after successful execution of retryHoldingCommandsInRequest().

[virajjasani]

@jonathan-hurley
Tried raising the concern in an old thread: https://groups.google.com/forum/#!topic/google-guice/UKWEjXEtj04 but no luck. At least the behavior doesn't seem to be working with our case.

Would you like to recommend removing @Transactional for retryHoldingCommandsInRequest() because that is not causing any different behavior than the current code? Please let me know if you have any other thoughts.
Thanks

[virajjasani]

Updated the PR with the changes requested. Tested and it is working fine.
Please review @jonathan-hurley

[asfgit]

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/Ambari-Github-PullRequest-Builder/5376/
Test PASSed.

[virajjasani]

Could you please review @jonathan-hurley

[virajjasani]

Thanks for the review @jonathan-hurley
Backport PR to branch-2.7: #3033

[virajjasani]

@jonathan-hurley if code freeze is not applied for 2.7.4 release, could you please help me merge this patch(trunk) and backport patch(branch-2.7)?