Ensure HMS shared lock is dropped on query failure

[losipiuk]

No description provided.

[findepi]

Is it testable?

[findepi]

It seems weird to commit transaction if our commit failed (ie when failure != null)

[losipiuk]

Yeah - I agree - though I am not sure if just changing this code to explicit abortTransaction here would be good too. Is there a chance that our commit did already partial work and it is too later for abort in HMS?

[findepi]

fortunately there is no "legacy behavior" -- we didn't commit/abort tx when our commit failed, so we can do whatever we want

i didn't test it, but abort seems way more appropriate... if we choose to call commit for some reason, we need to document why we think it's a good idea.

[findepi]

You could also extract the cleanup to a method, this would allow you to prove you're not swallowing Throwables

try {
 commit;
}
catch (Throwable commitFailure) {
  try {
    cleanup;
  } catch (Throwable cleanupFailure) {
    /* if ...*/ commitFailure.addSuppressed(cleanupFailure);
  }
  throw commitFailure; // rethrowing Throwable directly helps the compiler.... and future maintainers
}

// (commit succeeded)
cleanup;

[losipiuk]

reworked - looks nicer - thanks

[losipiuk]

Is it testable?

I can think of some UT like testing - but it is not a big benefit I think. I do not see a reasonable way to test it via integration/product test.

[findepi]

that was unused, right?

[losipiuk]

Yep