remove Firehose and FirehoseFactory#16758
Merged
clintropolis merged 5 commits intoapache:masterfrom Jul 19, 2024
Merged
Conversation
changes: * removed `Firehose` and `FirehoseFactory` and remaining implementations which were mostly no longer used after apache#16602 * Moved `IngestSegmentFirehose` which was still used internally by Hadoop ingestion to `DatasourceRecordReader.SegmentReader` * Rename `SQLFirehoseFactoryDatabaseConnector` to `SQLInputSourceDatabaseConnector` and similar renames for sub-classes * Moved anything remaining in a 'firehose' package somewhere else * Clean up docs on firehose stuff
github-actions
Bot
added
Area - Documentation
Area - Batch Ingestion
Area - Metrics/Event Emitting
Area - Segment Format and Ser/De
Area - Streaming Ingestion
Area - Dependencies
Kubernetes
Area - MSQ
9 tasks
kfaraz
reviewed
Jul 19, 2024
Contributor
kfaraz
left a comment
kfaraz
left a comment
There was a problem hiding this comment.
Thanks, @clintropolis !
I have left some minor suggestions, none of which are blockers for this PR.
With the exception of the renamed SegmentReader class, all the changes look good to me (I still need to go through SegmentReader).
| @JsonInclude(Include.NON_NULL) | ||
| @Deprecated | ||
| public FirehoseFactory getFirehoseFactory() | ||
| public IndexIOConfig(@Nullable Boolean appendToExisting, @Nullable Boolean dropExisting) |
Contributor
There was a problem hiding this comment.
Do we still need this constructor?
Member
Author
There was a problem hiding this comment.
good catch, removed
| * {@link org.apache.druid.server.security.AuthConfig#enableInputSourceSecurity} config. | ||
| * @throws UnsupportedOperationException if the given task type does not suppoert input source based security. Such | ||
| * would be the case, if the task uses firehose. | ||
| * @throws UnsupportedOperationException if the given task type does not suppoert input source based security |
Contributor
There was a problem hiding this comment.
I guess now it will never throw this exception.
Member
Author
There was a problem hiding this comment.
the default implementation still throws that exception, so i suppose if something doesn't implement this method it would happen
Comment on lines
+50
to
+56
| public ParallelIndexIOConfig(@Nullable Boolean appendToExisting) | ||
| { | ||
| this(firehoseFactory, null, null, appendToExisting, null); | ||
| this(null, null, appendToExisting, null); | ||
| } | ||
|
|
||
| @Deprecated | ||
| public ParallelIndexIOConfig(FirehoseFactory firehoseFactory, @Nullable Boolean appendToExisting, boolean dropExisting) | ||
| public ParallelIndexIOConfig(@Nullable Boolean appendToExisting, boolean dropExisting) |
Contributor
There was a problem hiding this comment.
We can completely remove these constructors now.
| } | ||
| } | ||
|
|
||
| public static class SegmentReader implements Closeable |
Contributor
There was a problem hiding this comment.
I think it would be easier to review this if we leave this class in a separate file and just rename IngestSegmentFirehose to SegmentReader.
Member
Author
There was a problem hiding this comment.
left this as an internal class btw because its only used internally to DatasourceRecordReader
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| public void testConnectorValidationInvalidUri() | ||
| { | ||
| derbyConnector = derbyConnectorRule.getConnector(); | ||
| final List<String> sqls = SqlTestUtils.selectFrom(TABLE_1, TABLE_2); |
Check notice
Code scanning / CodeQL
Unread local variable
sreemanamala
pushed a commit
to sreemanamala/druid
that referenced
this pull request
Aug 6, 2024
changes: * removed `Firehose` and `FirehoseFactory` and remaining implementations which were mostly no longer used after apache#16602 * Moved `IngestSegmentFirehose` which was still used internally by Hadoop ingestion to `DatasourceRecordReader.SegmentReader` * Rename `SQLFirehoseFactoryDatabaseConnector` to `SQLInputSourceDatabaseConnector` and similar renames for sub-classes * Moved anything remaining in a 'firehose' package somewhere else * Clean up docs on firehose stuff
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Mostly #16652 but without renaming
SinkandFireHydrant.changes:
FirehoseandFirehoseFactoryand remaining implementations which were mostly no longer used after Remove index_realtime and index_realtime_appenderator tasks #16602IngestSegmentFirehosewhich was still used internally by Hadoop ingestion toDatasourceRecordReader.SegmentReaderSQLFirehoseFactoryDatabaseConnectortoSQLInputSourceDatabaseConnectorand similar renames for sub-classes