Fixes discrepancy between admin fate delete and UserFateStore.delete#5016
Conversation
`admin fate delete` allows users to delete a transaction. If this transaction is a USER transaction, `UserFateStore.delete()` is how this is achieved. The admin code allows transactions with a status of SUBMITTED, IN_PROGRESS, NEW, FAILED, FAILED_IN_PROGRESS, SUCCESSFUL to call `delete()`, but the `delete()` code only allows NEW, SUBMITTED, SUCCESSFUL, FAILED. This commit fixes this discrepancy.
|
Regarding consistency - what's the behavior in earlier releases? I think they both should be consistent with that answer. |
Looking at the 3.1 code it would delete the fate tx no matter what state it was in. In 3.1 there as no check in the fate store of the status. I would recommend adding a forceDelete() call to the fate store that will delete no matter what the status. The admin code could call this and would have the same behavior as 3.1. However the non-admin fate code that calls delete would have this stronger check of the status that prevents bugs. Do not expect the non admin fate code to delete in progress transactions. |
Yeah, 3.1 and prior only has |
…tore.delete` Admin now calls a force delete to delete the transaction regardless of state. forceDelete() is a new method only to be used by Admin.
| void delete(); | ||
|
|
||
| /** | ||
| * Force remove the transaction from the store. Only to be used by {@link AdminUtil} |
There was a problem hiding this comment.
| * Force remove the transaction from the store. Only to be used by {@link AdminUtil} | |
| * Force remove the transaction from the store regardless of the status. Only to be used by {@link AdminUtil} |
|
|
||
| @Override | ||
| public void forceDelete() { | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
I like what this trying to do, but its far removed from the code that cares about this and if the code is refactored then this validation could be lost w/o anyone realizing it.
The following is a not a great solution, but its a solution. Could pass a boolean to the WrappedFateTxStore constructor that indicates if forceDelete should be allowed or not. This boolean could then cascade out to the code that creates the logging fate store. The boolean would always be false, but it links the code that really cares about this to this check and would force any refactorings to consider it.
Another solution is to add a comment exlaining why this throwing unsupported operation exception.
There was a problem hiding this comment.
Yeah, good point... I feel like comments are also likely to be overlooked/forgotten. I added the boolean in 9069a33. It seems like a fine solution.
There was a problem hiding this comment.
Those changes look good.
3 participants
admin fate deleteallows users to delete a transaction. If this transaction is a USER transaction,UserFateStore.delete()is how this is achieved. The admin code allows transactions with a status of SUBMITTED, IN_PROGRESS, NEW, FAILED, FAILED_IN_PROGRESS, SUCCESSFUL to calldelete(), but thedelete()code only allows NEW, SUBMITTED, SUCCESSFUL, FAILED. I changed theUserFateStore.delete()code to match the admin delete code. I'm not sure which one should be changed, but one of them should be.