Add tasks based ingestion #11
Conversation
vzayts
requested review from
tomasz-h2o,
zoido and
codyharris-h2o-ai
as code owners
There was a problem hiding this comment.
Thanks for the PR, couple of comments for your consideration.
Side question: do we really need this vast amount of structures introduced?
I understand that it covers thee parts of druid API we're using, though we're not really using big part of those fields, are we?
I'm not asking to remove them, though maybe make sure that we implement bare minumum for client to function.
vzayts
force-pushed
the
vzaytsev/add-tasks-based-ingestion
branch
from
76de2f7 to
bc46dc3
Compare
Most of these types were implemented previously, and in this PR they were moved from |
|
@vzaytsev1981 I just spotted the |
There was a problem hiding this comment.
Thanks for addressing my comments.
Apologies. As the PR is on the bigger side I keep spotting things that I did not spotted before as my attention was lead in different parts of the code.
I'll unblock as the changes I requested were done.
I still have doubts regarding:
- whether the metadata service fits the scope of the client/repo
- how to approach the default task spec
metadata.go
Outdated
| var datasourceAvailableQuery string | ||
|
|
||
| func fillDataSourceName(in string, ds string) string { | ||
| return strings.Replace(in, "${{ datasource }}", ds, 1) |
There was a problem hiding this comment.
I am still on the fence with this. I see SQL, I see potential injection and I feel uneasy about it.
Seeing it it opens more questions:
- Why do we use SQL and not native API?
- Does Metadata service even belongs here?
- The fact that it itself uses SQL and implements methods that feel more like application level helpers makes me doubt it.
@tomasz-h2o What i your taker on this? We have several iteration regarding some details that were outstanding enough that only now I see the forest.
There was a problem hiding this comment.
I un-exported the metadata service and its APIs, as well as other task-running helper functions. I also added the note on the intention for how metadata service is supposed to be used. Will that be OK @zoido, @tomasz-h2o ?
If that's not enough, I could refactor deeper to get rid of the service completely and move all the functionality to standalone test-helper functions.
There was a problem hiding this comment.
IMO that would be OK.
We could optionally add _test post-fix to filename, so that would sort of staple this in test only scope.
| @@ -43,10 +43,6 @@ func defaultTaskIngestionSpec() *TaskIngestionSpec { | |||
| Spec: &IngestionSpecData{ | |||
| DataSchema: &DataSchema{ | |||
| DataSource: "some_datasource", | |||
There was a problem hiding this comment.
IMHO, this can also be confusing. What about "unspecified" or something similar?
This pull request adds 2 new API services:
Tasks Service adds a different mode of ingestion - task-based, which is different from supervisor-based that runs ingestion tasks periodically. Three APIs added there for the tasks:
Submit,GetStatusandShutdown. Some of the ingestion spec types and functions were initially implemented for our integration tests by @tomasz-h2o. Now they are moved here and extended.Among other changes: extracting common ingestion specification data into
common_spec_types.go.I'd like to review this one against https://github.com/h2oai/go-druid/tree/tomasz/implement-suspend-resume that extends supervisor APIs implementation, and then have both features reviewed/merged/consumed for our migration data needs.