Refactor: Rename UsedSegmentChecker and cleanup task actions

[kfaraz]

Follow up to #16614

Changes

• Rename UsedSegmentChecker to PublishedSegmentsRetriever
• Remove deprecated single Interval argument from RetrieveUsedSegmentsAction
  as it is now unused and has been deprecated since support multiple intervals in dataSource inputSpec #1988
• Return Set of segments instead of a Collection from IndexerMetadataStorageCoordinator.retrieveUsedSegments()

[AmatyaAvadhanula]

Nit: Use CollectionUtils.isNullOrEmpty

[AmatyaAvadhanula]

Is there a style guide for this in Druid?
If not, I think that formatting can be subjective in the sense that a few people may prefer the previous code. (Not a strong opinion)

[kfaraz]

Fair point. I don't recall why I had made this change. Let me revert this with any other change that is required.

[kfaraz]

By the way, in terms of style guide, this is what I try to follow:

druid/codestyle/checkstyle.xml

Lines 252 to 296 in f1043d2

	According to the Druid code style, if a method or constructor declaration or a call

	doesn't fit a single line, each parameter or argument should be on it's own, e. g:

	

	MyReturnType myMethodWithAVeryLongName(

	MyParamTypeA myParamAWithAVeryLongName,

	MyParamTypeB myParamBWithAVeryLongName

	)

	

	or

	

	MyValueType myValue = myMethodWithAVeryLongName(

	myVeryLongArgA,

	myVeryLongArgB

	)

	

	The exceptions from this rule are map-like and pair-accepting constructors and methods,

	for those it's preferred to put each pair on it's own line, e. g:

	

	Map<MyKeyType, MyValueType> myMap = ImmutableMap.of(

	myKey1, myValue1,

	myKey2, myValue2

	)

	

	Always prefer to fit a declaration or a method or constructor call into a single line

	(less than 120 cols), if possible.

	

	Two things to note to avoid unnecessary breakdown:

	

	1) Exceptions declared for a method could be broken to the next line separately, e. g:

	

	MyReturnType myMethodWithAVeryLongName(MyParamTypeA myParamA, MyParamTypeB myParamB)

	throws MyExceptionTypeAWithVeryLongName, MyExceptionTypeBWithVeryLongName

	

	2) In a variable, field or constant assignment, it's often more readable to break the

	whole right hand side expression to the next line, instead of breaking the expression

	arguments, e. g:

	

	MyTypeWithAVeryLongName myVariableWithAVeryLongName =

	myMethodWithAVeryLongName(myArgA, myArgB);

	

	Also note that this checkstyle rule (the one that caused this message to be printed)

	doesn't spot all violations of the corresponding Druid code style rule. If you see

	a place where method or constructor parameters or call arguments are not properly

	located each on it's own line, but this checkstyle rule is silent, if doesn't mean

	that the code is formatted correctly. Fix it anyway.


Line 285 says:

2) In a variable, field or constant assignment, it's often more readable to break the 
whole right hand side expression to the next line, instead of breaking the expression

I also wrote something up here
#16563 (comment)

[kfaraz]

Thanks for the review, @AmatyaAvadhanula !

[cryptoe]

Left some comments.

[cryptoe]

Any specific reason to clean equals and hashcode ?

[kfaraz]

Instances of this class are never compared for equality.

[cryptoe]

I generally feel that is okay to have this since future developers might use this object in a map or some equality and then that's a bug.

[kfaraz]

I would agree if this was a POJO that was sent over the wire or even passed between classes. But this is meant only for the internal use of IndexerSqlMetadataStorageCoordinator and is exposed only for testing.

Better to have someone add it when they actually need it. Also, if someone is using this in a set or as the key of a map, they are doing something very wrong in the first place anyway.

The only real usage of equality for this class would be in tests but since that is not happening, the equals/hashCode methods were redundant.

[cryptoe]

We have essentially changed a long standing interface.
This might break custom extensions. Should we call this out in dev notes ?

[kfaraz]

I don't really feel so, because this interface is not exposed as an @ExtensionPoint or @PublicApi and extensions are not expected to implement this interface.

[cryptoe]

Fair enough.