-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
vdk-core: add memory properties client #921
Merged
Merged
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
In case Control Service is not available, for demo purposes, it would be good to be able to use some simple client so that jobs that stil use properties would work. To make it clear that it's not recommended for production (after all properties are supposed to be used to store state, so in memory client is very pointless) we write warning each time write proerties is called. Signed-off-by: Antoni Ivanov <[email protected]>
doks5
approved these changes
Aug 1, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, LGTM
projects/vdk-core/src/vdk/internal/builtin_plugins/job_properties/inmemproperties.py
Show resolved
Hide resolved
projects/vdk-core/src/vdk/internal/builtin_plugins/job_properties/properties_api_plugin.py
Outdated
Show resolved
Hide resolved
…ies/properties_api_plugin.py Co-authored-by: Andy <[email protected]>
antoniivanov
added a commit
that referenced
this pull request
Aug 2, 2022
* vdk-core: add memory properties client In case Control Service is not available, for demo purposes, it would be good to be able to use some simple client so that jobs that stil use properties would work. To make it clear that it's not recommended for production (after all properties are supposed to be used to store state, so in memory client is very pointless) we write warning each time write proerties is called. Signed-off-by: Antoni Ivanov <[email protected]>
duyguHsnHsn
pushed a commit
that referenced
this pull request
Aug 4, 2022
* vdk-core: add memory properties client In case Control Service is not available, for demo purposes, it would be good to be able to use some simple client so that jobs that stil use properties would work. To make it clear that it's not recommended for production (after all properties are supposed to be used to store state, so in memory client is very pointless) we write warning each time write proerties is called. Signed-off-by: Antoni Ivanov <[email protected]>
duyguHsnHsn
added a commit
that referenced
this pull request
Aug 8, 2022
* vdk-csv: add export-csv command * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * vdk-csv: modified requirements.txt * vdk-csv: modified requirements.txt * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * vdk-csv: changes on failing tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * vdk-csv: changes on failing tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * vdk-csv: changes on failing tests and adding examples * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * vdk-csv: changes on failing tests * cicd: increase memory limits for cicd runner (#927) what: We intend to increase the limits to our cicd containers.Updated values.yaml file. The script "install-runners.sh" has to be manually triggered for changes to take effect. why: Control Service Integration Tests have been failing with OOM errors. Although this doesn't directly update the values it servers as a reference point for future changes. testing: n/a Signed-off-by: Momchil Zhivkov [email protected] * versatile-data-kit: make easier slack instructions (#925) Add a link to Slack inviter (https://communityinviter.com/apps/cloud-native/cncf) to make it easier for people to join CNCF slack. Otherwise, they need to read the docs and it becomes more complex. Signed-off-by: Antoni Ivanov <[email protected]> * vdk-airflow: populate readme (#924) * control-service: increase integration test builder memory (#929) what: Increased the gradle builder/worker limits why: Integration tests keep failing with memory issues. testing: tests complete Signed-off-by: Momchil Zhivkov [email protected] * [pre-commit.ci] pre-commit autoupdate (#928) updates: - [github.com/asottile/pyupgrade: v2.37.2 → v2.37.3](asottile/pyupgrade@v2.37.2...v2.37.3) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Andy <[email protected]> * vdk-core: add memory properties client (#921) * vdk-core: add memory properties client In case Control Service is not available, for demo purposes, it would be good to be able to use some simple client so that jobs that stil use properties would work. To make it clear that it's not recommended for production (after all properties are supposed to be used to store state, so in memory client is very pointless) we write warning each time write proerties is called. Signed-off-by: Antoni Ivanov <[email protected]> * vdk-examples: add ingest and anonymize example (#922) * vdk-examples: add ingest and anonymize example The example was inspired by some talks with potential users. But regardless it's pretty interesting and useful example and added it to the collection of examples we have. We do not have a lot of examples of how plugins can be used and this shows they can be pretty powerful. Signed-off-by: Antoni Ivanov <[email protected]> * vdk-core: Improve ingestion error logging (#930) Currently, after ingestion completes, a user is presented with the following message: ``` Successful uploads:1 Failed uploads:0 ingesting plugin errors:defaultdict(<class 'vdk.internal.builtin_plugins.ingestion.ingester_utils.AtomicCounter'>, {}) ``` This is not ideal, as printing internal Python representations in user logs might be misleading, as a user might believe some error has occured when none did. This change fixes this by printing `None` instead when no errors occur, and printing the dictionary result instead of the defaultdict representation when errors did occur. ``` Successful uploads: 1 Failed uploads: 0 Ingesting plugin errors: None ``` Testing done: tested locally Signed-off-by: Gabriel Georgiev <[email protected]> * vdk-core,vdk-impala,vdk-lineage,vdk-trino: Support for pluggy 1.0 (#931) The 1.0 release of `pluggy` introduced a breaking change by renaming its `callers` module to `_callers`. This has been fixed by using the `HookCallResult` constant from `vdk.api.plugin.plugin_registry` instead of `pluggy.callers._Result` everywhere necessary, and amending said constant to be either `pluggy.callers._Result` or `pluggy._callers._Result` dynamically based on the version of `pluggy` in the current Python env. Also fixed a test which had `hookwrapper` set to True for some reason inside its testing plugin. Testing done: ran tests locally, CICD Signed-off-by: Gabriel Georgiev <[email protected]> * control-service: Atomic job cancellation (#860) Currently, there is a possibility for a job to be set to be cancelled, and have it complete before the cancellation can complete. This leads to a 500 error with no explanation. This change introduces an additional check that the operation response or its status is not null, and if it is, it logs an appropriate message and raises an appropriate exception. Testing done: unit tests Signed-off-by: Gabriel Georgiev <[email protected]> Co-authored-by: Miroslav Ivanov <[email protected]> Co-authored-by: Momchil Z <[email protected]> * vdk-csv: changes on unit tests * vdk-csv: changes on no data in database handling and new unit tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * vdk-impala, vdk-trino: Remove deprecated use of result field (#933) The _Result object from pluggy features a result field, which was deprecated in earlier versions in favour of get_result(), and removed entirely in 1.0. This change removes references to it which caused an error for a heartbeat test. Testing done: CICD Signed-off-by: Gabriel Georgiev <[email protected]> * vdk-csv: add helper methods and delete unused imports * vdk-csv: add helper methods and delete unused imports * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Momchil Z <[email protected]> Co-authored-by: Antoni Ivanov <[email protected]> Co-authored-by: Andy <[email protected]> Co-authored-by: Gabriel Georgiev <[email protected]> Co-authored-by: Miroslav Ivanov <[email protected]>
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.
In case Control Service is not available, for demo purposes, it would be
good to be able to use some simple client so that jobs that stil use
properties would work.
To make it clear that it's not recommended for production (after all
properties are supposed to be used to store state, so in memory client
may be misleading) we write warning each time write proerties is
called.
Signed-off-by: Antoni Ivanov [email protected]