Remove unused code and exception declarations#5461
Conversation
|
Thanks @leventov this is great housekeeping! I was surprised to learn that DataSegmentPuller isn't used anywhere 🌈⭐️ I don't see a reason to remove "redundant throws" in extension point interfaces. For example, this patch removes "throws Exception" in FirehoseV2's start method. Why? What's wrong with letting extensions throw exceptions there if they want to? I think this patch could be done in a non-incompatible way and it might even be better off like that. |
|
Okay, nevermind, it must be incompatible because of the removal of DataSegmentPuller. But I still am skeptical of the removal of |
|
@gianm this PR is going to be incompatible even if I return Exceptions to throws clauses at some places, because it removes |
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
I didn't read the entire diff, but I skimmed around and noticed a couple issues. Please double-check to see if there are any similar issues to the @JsonProperty one in RequestLogLine.
| @JsonProperty("timestamp") | ||
| public DateTime getCreatedTime() | ||
| { | ||
| return request.getTimestamp(); |
There was a problem hiding this comment.
This method should stay, since it's part of the serialized JSON (same goes for anything @JsonProperty).
There was a problem hiding this comment.
Reverted the method. There are no other occurrences when @JsonProperty is removed in this PR
| @@ -161,7 +161,7 @@ public ExecuteResult prepareAndExecute( | |||
| final String sql, | |||
| final long maxRowCount, | |||
| final PrepareCallback callback | |||
| ) throws NoSuchStatementException | |||
There was a problem hiding this comment.
Instead of deleting the throws NoSuchStatementException, probably getDruidStatement should be changed to throw NoSuchStatementException instead of IllegalStateException when the statement is not found.
There was a problem hiding this comment.
Made this change. Also note that authConfig is unused in DruidMeta, that might be a sign of some problem.
There was a problem hiding this comment.
Thanks. I guess don't worry about authConfig in this PR. I'll double check what's going on there separately.
| @@ -1,3 +1,7 @@ | |||
| <component name="DependencyValidationManager"> | |||
| <scope name="UnusedInspectionsScope" pattern="src[druid-processing]:*..*" /> | |||
| <scope name="UnusedInspectionsScope" pattern="src[java-util]:*..*" /> | |||
There was a problem hiding this comment.
this will need to changes if we merge #5490 first
| public interface FirehoseFactoryV2<T extends InputRowParser> | ||
| { | ||
| FirehoseV2 connect(T parser, Object lastCommit) throws IOException, ParseException; | ||
| FirehoseV2 connect(T parser, Object lastCommit) throws ParseException; |
There was a problem hiding this comment.
am wondering why we are removing IOException, seems reasonable that such method will throw IOException?
There was a problem hiding this comment.
Because nobody actually throwing it
There was a problem hiding this comment.
But it is legitimate kind of exception that maybe in the future or other external implementations will need it, FirehoseFactoryV2.java is an extension point, IMO we should keep it.
|
👍 after tests. |
|
Thanks for reviews |
|
I was using |
|
was also using some of the methods in |
|
And MapUtils. getLong |
|
This PR will need a special callout in the release notes that a lot of java-util functions were removed. |
5 participants
Removes a lot, but still not all unused code in the project.
Labelled
Incompatiblebecause removes some exceptions from throws clauses of some methods in@ExtensionPointinterfaces.