FateOpsCommandsIT enhancements #4400
Conversation
- Added test to verify name and step of transactions using summary and print commands - Added check to filter by non-existent FateId in summary and print command tests - Some other small misc improvemements
| @@ -92,6 +93,18 @@ private List<String> formatLockInfo(final List<String> lockInfo, | |||
| return formattedLocks; | |||
| } | |||
|
|
|||
| public long getRunning() { | |||
There was a problem hiding this comment.
I'm thinking that the FateTxnDetails variables should be marked final and only initialized in the constructor.
| @@ -100,6 +113,14 @@ public String getStatus() { | |||
| return status; | |||
| } | |||
|
|
|||
| public List<String> getLocksHeld() { | |||
| return Collections.unmodifiableList(locksHeld); | |||
There was a problem hiding this comment.
List.of returns an unmodifiable List, I don't think we need to wrap it again in the case that it's not set. Instead of initializing the locksHeld and locksWaiting variables with List.of, you could mark them final and initialize them in the constructor with an unmodifiable list.
|
The constructor may return before anything is initialized, so I don't believe the fields can be final. I did change locksHeld and locksWaiting to be declared unmodifiable in the constructor though |
|
I realize that this is contradictory to Dave's comment and the latest changes - likely its a matter of preference. Maybe updating (adding) the javadocs for those methods calling out they return an unmodified list could help? Wrapping the list with unmodifiable in the method makes it clear that the list is not expected to be changed by the caller. Moving that to the constructor means that you'd need to dig for that info if it ever was important. I do not know what the best practices are for making / marking things immutable - but anything that would help reason about thread safety seems beneficial. With this class, and the way that it's used it may not matter much, but it may be something we should look to adopt / strive for. This is likely a personal preference and can be ignored with this PR - but I did want to add this comment to prompt that we should be thinking about documenting thread safety as we modify code. |
- Changed result.contains("0 transactions") to result.contains(" 0 transactions") to avoid returning true if the number of transactions is not 0 but ends in 0
- Added shutting down the compactor to BeforeEach (this also has the effect of adding this shutdown to testTransactionNameAndStep() which I mistakenly thought shouldn't shutdown the compactor)
- Simplified testTransactionNameAndStep() (unneccessary config and checks). Also added deleting the table at the end of the test.
This addresses #4350 (review)