Avoid race-conditions while executing sub-commands #1201
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
`executor.Exec` now uses `cmd.CombinedOutput`. Previous implementation was hanging on my windows VM during `bundle deploy` on the `ReadAll(MultiReader(stdout, stderr))` line. Seems to be related to the fact the MultiReader reads sequentially. Also noticed that Exec was not removing `scriptFile` after itself, fixed that too.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1201 +/- ##
==========================================
- Coverage 51.93% 51.91% -0.03%
==========================================
Files 299 299
Lines 16605 16607 +2
==========================================
- Hits 8624 8621 -3
- Misses 7353 7357 +4
- Partials 628 629 +1 ☔ View full report in Codecov by Sentry. |
andrewnester
approved these changes
Feb 12, 2024
andrewnester
added a commit
that referenced
this pull request
Feb 15, 2024
CLI: * Ignore environment variables for `auth profiles` ([#1189](#1189)). * Update LICENSE ([#1013](#1013)). Bundles: * Added `bundle deployment bind` and `unbind` command ([#1131](#1131)). * Use allowlist for Git-related fields to include in metadata ([#1187](#1187)). * Added `--restart` flag for `bundle run` command ([#1191](#1191)). * Generate correct YAML if custom_tags or spark_conf is used for pipeline or job cluster configuration ([#1210](#1210)). Internal: * Move folders package into libs ([#1184](#1184)). * Log time it takes for profile to load ([#1186](#1186)). * Use mockery to generate mocks compatible with testify/mock ([#1190](#1190)). * Retain partially valid structs in `convert.Normalize` ([#1203](#1203)). * Skip `for_each_task` when generating the bundle schema ([#1204](#1204)). * Regenerate the CLI using the same OpenAPI spec as the SDK ([#1205](#1205)). * Avoid race-conditions while executing sub-commands ([#1201](#1201)). API Changes: * Changed `databricks connections delete` command with new required argument order. * Changed `databricks connections get` command with new required argument order. * Changed `databricks connections update` command with new required argument order. * Added `databricks tables exists` command. * Changed `databricks volumes delete` command with new required argument order. * Changed `databricks volumes read` command with new required argument order. * Changed `databricks volumes update` command with new required argument order. * Added `databricks lakehouse-monitors` command group. * Removed `databricks files get-status` command. * Added `databricks files create-directory` command. * Added `databricks files delete-directory` command. * Added `databricks files get-directory-metadata` command. * Added `databricks files get-metadata` command. * Added `databricks files list-directory-contents` command. * Removed `databricks pipelines reset` command. * Changed `databricks account settings delete-personal-compute-setting` command with new required argument order. * Removed `databricks account settings read-personal-compute-setting` command. * Changed `databricks account settings update-personal-compute-setting` command with new required argument order. * Added `databricks account settings get-personal-compute-setting` command. * Removed `databricks settings delete-default-workspace-namespace` command. * Removed `databricks settings read-default-workspace-namespace` command. * Removed `databricks settings update-default-workspace-namespace` command. * Added `databricks settings delete-default-namespace-setting` command. * Added `databricks settings delete-restrict-workspace-admins-setting` command. * Added `databricks settings get-default-namespace-setting` command. * Added `databricks settings get-restrict-workspace-admins-setting` command. * Added `databricks settings update-default-namespace-setting` command. * Added `databricks settings update-restrict-workspace-admins-setting` command. * Changed `databricks token-management create-obo-token` command with new required argument order. * Changed `databricks token-management get` command to return . * Changed `databricks clean-rooms delete` command with new required argument order. * Changed `databricks clean-rooms get` command with new required argument order. * Changed `databricks clean-rooms update` command with new required argument order. * Changed `databricks dashboards create` command . New request type is . * Added `databricks dashboards update` command. OpenAPI commit c40670f5a2055c92cf0a6aac92a5bccebfb80866 (2024-02-14) Dependency updates: * Bump github.com/hashicorp/hc-install from 0.6.2 to 0.6.3 ([#1200](#1200)). * Bump golang.org/x/term from 0.16.0 to 0.17.0 ([#1197](#1197)). * Bump golang.org/x/oauth2 from 0.16.0 to 0.17.0 ([#1198](#1198)). * Bump github.com/databricks/databricks-sdk-go from 0.30.1 to 0.32.0 ([#1199](#1199)).
Merged
github-merge-queue bot
pushed a commit
that referenced
this pull request
Feb 15, 2024
CLI: * Ignore environment variables for `auth profiles` ([#1189](#1189)). * Update LICENSE file to match Databricks license language ([#1013](#1013)). Bundles: * Added `bundle deployment bind` and `unbind` command ([#1131](#1131)). * Use allowlist for Git-related fields to include in metadata ([#1187](#1187)). * Added `--restart` flag for `bundle run` command ([#1191](#1191)). * Generate correct YAML if `custom_tags` or `spark_conf` is used for pipeline or job cluster configuration ([#1210](#1210)). Internal: * Move folders package into libs ([#1184](#1184)). * Log time it takes for profile to load ([#1186](#1186)). * Use mockery to generate mocks compatible with testify/mock ([#1190](#1190)). * Retain partially valid structs in `convert.Normalize` ([#1203](#1203)). * Skip `for_each_task` when generating the bundle schema ([#1204](#1204)). * Regenerate the CLI using the same OpenAPI spec as the SDK ([#1205](#1205)). * Avoid race-conditions while executing sub-commands ([#1201](#1201)). API Changes: * Added `databricks tables exists` command. * Added `databricks lakehouse-monitors` command group. * Removed `databricks files get-status` command. * Added `databricks files create-directory` command. * Added `databricks files delete-directory` command. * Added `databricks files get-directory-metadata` command. * Added `databricks files get-metadata` command. * Added `databricks files list-directory-contents` command. * Removed `databricks pipelines reset` command. * Changed `databricks account settings delete-personal-compute-setting` command with new required argument order. * Removed `databricks account settings read-personal-compute-setting` command. * Changed `databricks account settings update-personal-compute-setting` command with new required argument order. * Added `databricks account settings get-personal-compute-setting` command. * Removed `databricks settings delete-default-workspace-namespace` command. * Removed `databricks settings read-default-workspace-namespace` command. * Removed `databricks settings update-default-workspace-namespace` command. * Added `databricks settings delete-default-namespace-setting` command. * Added `databricks settings delete-restrict-workspace-admins-setting` command. * Added `databricks settings get-default-namespace-setting` command. * Added `databricks settings get-restrict-workspace-admins-setting` command. * Added `databricks settings update-default-namespace-setting` command. * Added `databricks settings update-restrict-workspace-admins-setting` command. * Changed `databricks token-management create-obo-token` command with new required argument order. * Changed `databricks token-management get` command to return . * Changed `databricks dashboards create` command . New request type is . * Added `databricks dashboards update` command. OpenAPI commit c40670f5a2055c92cf0a6aac92a5bccebfb80866 (2024-02-14) Dependency updates: * Bump github.com/hashicorp/hc-install from 0.6.2 to 0.6.3 ([#1200](#1200)). * Bump golang.org/x/term from 0.16.0 to 0.17.0 ([#1197](#1197)). * Bump golang.org/x/oauth2 from 0.16.0 to 0.17.0 ([#1198](#1198)). * Bump github.com/databricks/databricks-sdk-go from 0.30.1 to 0.32.0 ([#1199](#1199)). --------- Co-authored-by: Pieter Noordhuis <pieter.noordhuis@databricks.com>
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.
Changes
executor.Execnow usescmd.CombinedOutput. Previous implementation was hanging on my windows VM duringbundle deployon theReadAll(MultiReader(stdout, stderr))line.The problem is related to the fact the MultiReader reads sequentially, and the
stdoutis the first in line. Even simpleio.ReadAll(stdout)hangs on me, as it seems like the command that we spawn (python wheel build) waits for the error stream to be finished before closing stdout on its own side? Readingstderr(orout) in a separate go-routine fixes the deadlock, butcmd.CombinedOutputfeels like a simpler solution.Also noticed that Exec was not removing
scriptFileafter itself, fixed that too.Tests
Unit tests and manually