Conversation
To allow users to quickly parsing and schema, add a firehose that reads data that is inlined in its spec.
ccaominh
force-pushed
the
feature-656-inline-firehose
branch
from
b2a9abe to
99fa288
Compare
|
❤️ ❤️ ❤️ ❤️ ❤️ |
|
Manual test: |
|
Man, the |
gianm
left a comment
gianm
left a comment
There was a problem hiding this comment.
LGTM, just had a comment about the Sonar annotations.
clintropolis
left a comment
clintropolis
left a comment
There was a problem hiding this comment.
overall lgtm 👍
I don't consider any of my comments as blockers to merge, just pointing out some things I noticed so feel free to ignore
| ### SqlFirehose | ||
|
|
||
| SqlFirehoseFactory can be used to ingest events residing in RDBMS. The database connection information is provided as part of the ingestion spec. For each query, the results are fetched locally and indexed. If there are multiple queries from which data needs to be indexed, queries are prefetched in the background upto `maxFetchCapacityBytes` bytes. | ||
| This Firehose can be used to ingest events residing in RDBMS. The database connection information is provided as part of the ingestion spec. For each query, the results are fetched locally and indexed. If there are multiple queries from which data needs to be indexed, queries are prefetched in the background upto `maxFetchCapacityBytes` bytes. |
There was a problem hiding this comment.
nit: i know you didn't write this, but maybe it should be "an RDBMS."?
| This firehose can be used to combine and merge data from a list of different firehoses. | ||
| This can be used to merge data from more than one firehose. | ||
| This Firehose can be used to combine and merge data from a list of different Firehoses. | ||
| This can be used to merge data from more than one Firehose. |
There was a problem hiding this comment.
Same thing about not writing, but this 2nd sentence seems redundant
| import java.util.function.Predicate; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class FirehoseModuleTest |
There was a problem hiding this comment.
I don't quite understand the point of this test, is it just seeing .. if jackson works I guess for the things that are defined in the module? It doesn't really test all firehose factories since some are registered in extensions, am I missing something?
There was a problem hiding this comment.
The idea is that this test fails if the newInlineFirehoseFactory is not registered in FirehoseModule, for example (or any other FirehoseFactories in that package). Are there tests elsewhere that would catch an error like that?
There was a problem hiding this comment.
Ah I guess in the case of tests you've added, no nothing else would be testing this. I thought that generally the 'serde' tests are usually setup to catch this, but looking closer this isn't terribly consistent across the codebase either.
6 participants
Fixes #7910.
Description
To allow users to quickly parsing and schema, add a firehose that reads data that is inlined in its spec.
This PR has: