accumulo icon indicating copy to clipboard operation
accumulo copied to clipboard

Published 20 hours ago • verified publisher icon apache

FateOpsCommandsIT enhancements

Open kevinrr888 opened this issue 11 months ago • 3 comments

This addresses https://github.com/apache/accumulo/pull/4350#pullrequestreview-1929324130

  • 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

kevinrr888 avatar Mar 18 '24 20:03 kevinrr888

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

kevinrr888 avatar Mar 20 '24 15:03 kevinrr888

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.

EdColeman avatar Mar 20 '24 15:03 EdColeman

Made some misc changes to this PR that I realized from working on https://github.com/apache/accumulo/issues/4462 (see https://github.com/apache/accumulo/pull/4400/commits/05fffe2a9bec75c92d3623b59314e15024517520)

kevinrr888 avatar May 13 '24 21:05 kevinrr888

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

Looking at the constructor the way it is currently structured it would require some sort of refactoring to use final. That could be a nice improvement in a follow on PR.

keith-turner avatar Jun 04 '24 15:06 keith-turner