druid
druid copied to clipboard
apache
Removes FiniteFirehoseFactory and its implementations
Description
The FiniteFirehoseFactory and InputRowParser classes were deprecated in 0.17.0 (https://github.com/apache/druid/pull/8823) in favor of InputSource & InputFormat. This PR removes the FiniteFirehoseFactory and all its implementations along with classes solely used by them like Fetcher (Used by PrefetchableTextFilesFirehoseFactory). Refactors classes including tests using FiniteFirehoseFactory to use InputSource instead.
Removing InputRowParser may not be as trivial as many classes that aren't deprecated depends on it (with no alternatives), like EventReceiverFirehoseFactory. Hence FireHoseFactory, EventReceiverFirehoseFactory, and Firehose are marked deprecated.
Discussion thread: https://lists.apache.org/thread/lq5846jy7j4kf6r379cy4gpgqzdkfsf3
Fixed the bug in AbstractParallelIndexSupervisorTaskTest#compareTaskReports(..)
Indexing Task tests assert generated reports are as expected using compareTaskReports(..). This becomes flaky in the case of Sequential Index Task since the generated movingAverages field in reports varies with task execution time. This PR modifies the method to skip this field.
Renamed the class TestRealtimeTask to TestIndexTask
Only RemoteTaskRunnerTest & WorkerTaskMonitorTest uses RealtimeTask currently. Modifying the name to TestIndexTask and implementing IndexTask (to use InputSource instead of FirehoseFactory) shouldn't affect both tests as they're simply testing RemoteTaskRunner & WorkerTaskMonitor functionalities, and they are task agnostic.
Removed ITCombiningFirehoseFactoryIndexTest
Since combining input source isn't supported by the IndexTask and ITCombiningInputSourceParallelIndexTest already exists, removing ITCombiningFirehoseFactoryIndexTest in this PR.
This PR has:
- [x] been self-reviewed.
- [ ] using the concurrency checklist (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [x] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in licenses.yaml
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
- [ ] added integration tests.
- [x] been tested in a test Druid cluster.
This pull request fixes 1 alert when merging d11e740724b43f137a0afa942f53fb5587be949e into eabce8a1590d564702321fea98157289776e63c5 - view on LGTM.com
fixed alerts:
- 1 for Server-side request forgery
![lgtm-com[bot] avatar](https://avatars.githubusercontent.com/in/17324?v=4 "Published by a pub.dev verified publisher"){kind=small}
Thanks much for making this fix. An impressive amount of code is deleted in this PR!
I wonder, will this fix the many issues with PrefetchableTextFilesFirehoseFactoryTest? It seems that one fails about 10-20% of the time for me in Travis builds.
Hi @paul-rogers, This PR removes PrefetchableTextFilesFirehoseFactory and its test entirely since it's an implementation of FiniteFirehoseFactory.
Does anything need to be removed from the docs? Is that going to be done in a separate PR?
This pull request fixes 1 alert when merging 10ac5b31d502398d223e4350f13d90dbff8f2e19 into 2912a36a202672b2b025ef447c5bcbded7b20990 - view on LGTM.com
fixed alerts:
- 1 for Server-side request forgery
We also had an offline discussion about MultiPhaseParallelIndexingRowStatsTest, which failed in one of my PRs. I see that the base class, AbstractMultiPhaseParallelIndexingTest, is adjusted in this PR, but MultiPhaseParallelIndexingRowStatsTest is not changed. Just want to confirm that this PR will fix the issue with that test: that we are comparing the time-based stats against expected values, and that the test randomly fails as a result since we can't guarantee execution duration.
Hi, @vogievetsky, there are a couple of places that need to be changed in native-batch-firehose.md, native-batch.md, few extension configurations. It'll be done in current PR itself. Thanks for notifying.
Hi, @paul-rogers, We were comparing the generated task reports with time-based stats fields i.e. movingAverages against expected values, which is ignored during comparison with this PR and will not fail for any parallel indexing task test using this method to compare reports.
LGTM (non-binding)
Would love to see this gets merged as this will unblock PRs that are failing builds due to the tests which this PR fixes.
This pull request fixes 1 alert when merging 76fd722a1f1665ce4b06d33e16471b68e6c27970 into 2855fb6ff8181d83fa679db26c8bbddf839391fd - view on LGTM.com
fixed alerts:
- 1 for Server-side request forgery
thank you for explaining @tejaswini-imply. I understand it better now and I am at more ease 😄 can you look into the build failures? Would like to see a green build before giving +1 on this.
This pull request fixes 1 alert when merging 62a32621ebc6bf03e9d22fd75435a5d26a4f4eef into b4985ccd5e9b10fec847b3316259fc97996eebca - view on LGTM.com
fixed alerts:
- 1 for Server-side request forgery
This pull request fixes 1 alert when merging 6c3e09f486fa2c9909c66f73524c115270336abe into b4985ccd5e9b10fec847b3316259fc97996eebca - view on LGTM.com
fixed alerts:
- 1 for Server-side request forgery
This pull request fixes 1 alert when merging 01e0c90a104ad79f65d20c0cd4ed180568d3de60 into 38af5f7b57769e026f028a56bb9be8003e01ddc5 - view on LGTM.com
fixed alerts:
- 1 for Server-side request forgery
This pull request fixes 1 alert when merging c9f816ccd6c0433faf286db5bce28a19f483186c into 38af5f7b57769e026f028a56bb9be8003e01ddc5 - view on LGTM.com
fixed alerts:
- 1 for Server-side request forgery
This pull request fixes 1 alert when merging 2aa663982a51578fa3b7f68da56223a8d6b9ecf1 into 38af5f7b57769e026f028a56bb9be8003e01ddc5 - view on LGTM.com
fixed alerts:
- 1 for Server-side request forgery
This pull request fixes 1 alert when merging a1a9ce65c57285b350e1451beb69ef76ccf13106 into 3a3271eddc946599e1c9b81154d2f37663b186c5 - view on LGTM.com
fixed alerts:
- 1 for Server-side request forgery
I think the compaction integration tests might be legitimately failing (i restarted them all last night and still not passing). I haven't looked closer yet to see what the issue might be.
This pull request fixes 1 alert when merging 5eda0b418e9088ac81fadb45faf70166baed799d into ec8bdeb9f6f2aac28e78eb41df210da7e74370bb - view on LGTM.com
fixed alerts:
- 1 for Server-side request forgery
@clintropolis I have tested it locally, and the compaction IT group tests or the modified wikipedia_with_timestamp_index_task.json, wikipedia_index_task_with_dimension_spec.json index task spec seems to have no issues. I am not sure what's causing the failures in Travis, though.
This pull request fixes 1 alert when merging ed89707b3eacfc8fc4d5fb29a412b478a121340b into 0d7bf66578479afd920ba96db4c44a01e912833b - view on LGTM.com
fixed alerts:
- 1 for Server-side request forgery