Add Ingestion Job management API for Feast Core (#548) #624
Conversation
This file contains 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
khorshuheng
requested review from
davidheryanto,
pradithya,
woop and
zhilingc
as code owners
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: khorshuheng The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
* Added protobuf definitions for a Job Management API * Added query methods to JobRepository to query jobs by store and featureset * Added hashCode() & equals() to Job model compare and hash jobs This would allow jobs to be used as elements in HashSets and keys in HashMaps * Added toIngestionProto() to Job object to convert Job model to ingestion job proto * Added query methods to FeatureSetRepository to query by exact (name, project) or (name,version) * Added listJobs() to JobService to handle request list ingestion jobs requests * Added missing filter field in ListIngestionJobsRequest protobuf * Added code to setup mockups for testing JobService * Revert "Added hashCode() & equals() to Job model compare and hash jobs" This reverts commit ba995bb712cbd38f0f4bef47efbe433d5ec07521 as it caused core tests to fail * Added JobServiceTest unit tests for JobService's listJobs() * Added stopJobs() to JobService to handle requests to stop jobs * Added getTransitionalStates() to JobStatus to return collection of transitional states * Changed stopJobs() to throw unsupported error on transitional job statuses * Moved conversion of JobStatus to IngestionJobStatus proto to JobStatus. * Make findFeatureSet() match only one featureset. Limit findFeatureSets() as feature set references as composite keys should match one and only one featureset. * Added matchFeatureSets() to SpecService to match Feature Sets from References This commit is adds temporary support for FeatureSetReference to SpecService via listFeatureSets(). In the future, this should merged together with listFeatureSets() as their functionality is almost the same. * Refactor JobService listJobs() to use SpecService to provide featureset matching * Revert findBy methods added to FeatureSetRepository as no longer used. JobService no longer depends on FeatureSetRepository directly, instead via SpecService * Added restartJob() to JobManagers to restart ingestion/import jobs * Added restartJob() to JobService to restart ingestion jobs * Update job model (due to new extId) when restartJob() in JobService * Use assertThat() & equalTo() instead of assertEquals() in JobService * Revert "Use assertThat() & equalTo() instead of assertEquals() in JobService" due to failed tests This reverts commit bb3cf0ae1e7cfbde7a2f997cacc661d040c0a35f. * Use hamcrest’s assertThat() and equalTo() instead of Junit's assertEquals * Hook up JobService list, stop, restart job methods to CoreServiceImpl. * Throw InvalidArgumentException instead when calling listJobs() with invalid FeatureSetReference Throw InvalidArgumentException instead of UnsupportedOperationException for better semantics: UnsupportedOperationException should be reserved for operations that fail due to failed preconditions * Fixed typos in javadocs: InvalidArgumentException should be IllegalArgumentException * Fixed in findByFeatureSetsIn() query in JobRepository * Renamed toIngestionProto() methods to toProto() to follow code convention * Make JobService's listJobs() a transaction to prevent DB data race conditions * Moved matchFeatureSets() in SpecService to private method in JobService * Make the map that maps between JobStatus and IngestionJobStatus static * Make JobService's listJobs() to return all ingestion jobs on empty filter * Fixed issue where the jobManager map that JobService built used wrong keys * Fix issue where actual Job Status is not synced with database. Issue occurs when the job is aborted/restarted, but the JobStatus has not yet been updated by JobUpdateTask. Hence another call to abortJob() & restartJob() that should be rejected due invalid status is allowed through * Log stopJob() & restartJob() operations to make debugging easier * Use Runner.name() instead of runner.toString() to build JobManager map * Move documentation on JobService operations to CoreService protobuf definition * Added IngestJob to python sdk as native representation of IngestionJob proto * Make empty filter on JobService's listJobs() select all ingestion jobs * Added bindings for Job management API to python sdk client. * Fixed __connect_core() to connect to Feast CoreService when calling on Job API calls * Auto reload IngestJob.status and IngestJob.external_id on get property. * Added IngestJob.wait() to wait for job status to transtion * Added basic job api e2e test to exercise job api * Reorder the operations e2etest to make sure that jobs are running after test * Added e2e test for all types exercising job api * Fixed typo in function arguments * Added unit tests for Ingestion Job API additions in python sdk * Rename "ingestion" to "ingest" for more consistent naming * Disable support for restarting Job in a terminal state due to possible race conditions * Added __str__ and __repr__ to IngestJob to render ingestjob in human readable string * Added FeatureSetRef to represent references to featursets * Admend client's list_ingest_jobs() to accept feature references directly * Fixed typo in IngestJob.store property * Fixed issue with FeatureSetRef.from_str not converting version to int * Make the grpc error message more apparent on stop_ingest_job() and restart_ingest_job() * Added feast ingest-job list, describe, stop, restart to CLI * Rename Job to RetrievalJob to prevent confusion with IngestJob * Updated e2e tests to use FeatureSetRef in list_ingest_jobs() * Fixed due e2e tests to cater to new limitations on stop_ingest_job() * Increase timeout on test_all_types_ingest_jobs() e2e test. * Configure IngestJob.wait() to backoff with a exponentially larger wait duration * Added print statements to debug e2e test failure. * Revert "Added print statements to debug e2e test failure." This reverts commit 146fb2b. * Fixed issue of test waiting for aborted job to become running causing timeout. Co-authored-by: Zhu Zhanyan <zhu.zhanyan@gojek.com>
khorshuheng
force-pushed
the
backport-548
branch
from
efe77c7
to
5503d7a
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Backport #548