-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor: Rename UsedSegmentChecker and cleanup task actions #16644
Conversation
theIntervals = ImmutableList.of(interval); | ||
} else if (intervals != null && intervals.size() > 0) { | ||
theIntervals = JodaUtils.condenseIntervals(intervals); | ||
if (intervals == null || intervals.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Use CollectionUtils.isNullOrEmpty
null | ||
); | ||
final SegmentTransactionalInsertAction action = SegmentTransactionalInsertAction | ||
.overwriteAction(null, ImmutableSet.of(SEGMENT3), null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point. I don't recall why I had made this change. Let me revert this with any other change that is required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Thanks for the review, @AmatyaAvadhanula ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments.
...rvice/src/main/java/org/apache/druid/indexing/common/actions/RetrieveUsedSegmentsAction.java
Show resolved
Hide resolved
@@ -3022,34 +3012,5 @@ public String getErrorMsg() | |||
{ | |||
return errorMsg; | |||
} | |||
|
|||
@Override | |||
public boolean equals(Object o) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason to clean equals and hashcode ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instances of this class are never compared for equality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
*/ | ||
default Collection<DataSegment> retrieveUsedSegmentsForInterval( | ||
default Set<DataSegment> retrieveUsedSegmentsForInterval( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have essentially changed a long standing interface.
This might break custom extensions. Should we call this out in dev notes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
Follow up to #16614
Changes
UsedSegmentChecker
toPublishedSegmentsRetriever
Interval
argument fromRetrieveUsedSegmentsAction
as it is now unused and has been deprecated since support multiple intervals in dataSource inputSpec #1988
Set
of segments instead of aCollection
fromIndexerMetadataStorageCoordinator.retrieveUsedSegments()