Skip to content
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

[exec,lib] partial revert of using main_session for KlioConfig #147

Merged
merged 1 commit into from Jan 14, 2021

Conversation

DanSimon
Copy link
Contributor

@DanSimon DanSimon commented Jan 12, 2021

This is a partial revert for f58caf7.

In some cases for unknown reasons, RunConfig.get() will begin raising an exception that the config is no longer set. This should not be happening, and in some instances the problem doesn't show up until a streaming job has been running for hours, but there is likely some underlying issue in Beam/Dataflow that is leading the main session variable to be deleted.

This PR is not intended to be a complete fix, but rather a mitigation that reverts back to klioexec getting its config via the effective-klio-job.yaml that should be included with the packaged code.

I've left the klio-cli changes untouched, meaning it will continue to write a temp file and bind it for use by klioexec. klioexec will also still set the KlioConfig to the main session variable with RunConfig.set(), even though for now it won't be used.

Checklist for PR author(s)

  • Format the pull request title like [cli] Fixes bugs in 'klio job fake-cmd'.
  • Changes are covered by unit tests (no major decrease in code coverage %) and/or integration tests.
  • Document any relevant additions/changes in the appropriate spot in docs/src.

@DanSimon DanSimon force-pushed the dsimon/undo-exec-main-session-config branch from 410eb84 to 192f5b5 Compare January 13, 2021 22:20
@DanSimon DanSimon force-pushed the dsimon/undo-exec-main-session-config branch from 192f5b5 to a57879c Compare January 13, 2021 22:33
# TODO : remove this when the call in spotify-klio is removed
def _compare_runtime_to_buildtime_config(runtime_config_path):
return True
def _compare_runtime_to_buildtime_config(klio_config):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 changes from how this function used to work:

  1. It used to just return True if it couldn't find the build config, apparently to be compatible with old versions that didn't rely on it. I've removed this.
  2. The check used to just hash files, but since the temp-written config file from klio-cli can be slightly different from what it reads in, it was easier to just create a second KlioConfig and compare their dicts.

_thread_local = threading.local()

@classmethod
def _load_config_from_file(cls):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing a straight revert, I opted to keep RunConfig around and move this method into it, which will hopefully better encapsulate subsequent changes we make to this logic.

@DanSimon DanSimon changed the title WIP : [exec,lib] partial revert of using main_session for KlioConfig [exec,lib] partial revert of using main_session for KlioConfig Jan 14, 2021
Copy link
Contributor

@fallonchen fallonchen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@DanSimon DanSimon merged commit 5847c7e into master Jan 14, 2021
@DanSimon DanSimon deleted the dsimon/undo-exec-main-session-config branch January 14, 2021 17:36
econchick added a commit that referenced this pull request Jan 25, 2021
This was missed when doing PR #147
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants