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

Fix databricks plugin #4206

Merged
merged 4 commits into from
Oct 17, 2023
Merged

Fix databricks plugin #4206

merged 4 commits into from
Oct 17, 2023

Conversation

pingsutw
Copy link
Member

@pingsutw pingsutw commented Oct 12, 2023

Tracking issue

The databricks plugin does not check for all possible terminal states as returned from the Databricks Jobs API. As a result, if the Databricks jobs API returns a state which the plugin is not written to recognise, the plugin will think that the job is still in a running phase.

#4243

Describe your changes

Transition task status to Failure if the lifeCycleState is SKIPPED or INTERNAL_ERROR

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Screenshots

Note to reviewers

Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw pingsutw marked this pull request as draft October 12, 2023 06:41
@codecov
Copy link

codecov bot commented Oct 12, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (0ca2d22) 58.95% compared to head (72e1261) 59.36%.
Report is 17 commits behind head on master.

❗ Current head 72e1261 differs from pull request most recent head 72b5850. Consider uploading reports for the commit 72b5850 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4206      +/-   ##
==========================================
+ Coverage   58.95%   59.36%   +0.40%     
==========================================
  Files         621      552      -69     
  Lines       52932    39905   -13027     
==========================================
- Hits        31206    23688    -7518     
+ Misses      19229    13884    -5345     
+ Partials     2497     2333     -164     
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...atacatalog/pkg/manager/impl/artifact_data_store.go 68.00% <ø> (+8.00%) ⬆️
datacatalog/pkg/manager/impl/artifact_manager.go 70.23% <ø> (+1.13%) ⬆️
datacatalog/pkg/manager/impl/dataset_manager.go 68.69% <ø> (+2.02%) ⬆️
...atacatalog/pkg/manager/impl/reservation_manager.go 81.30% <ø> (+2.84%) ⬆️
datacatalog/pkg/manager/impl/tag_manager.go 70.21% <ø> (+0.56%) ⬆️
datacatalog/pkg/repositories/config/postgres.go 64.51% <ø> (-0.35%) ⬇️
datacatalog/pkg/repositories/factory.go 0.00% <ø> (ø)
datacatalog/pkg/repositories/gormimpl/artifact.go 84.37% <ø> (+1.44%) ⬆️
datacatalog/pkg/repositories/gormimpl/dataset.go 83.72% <ø> (+0.70%) ⬆️
datacatalog/pkg/repositories/gormimpl/list.go 68.42% <ø> (+3.03%) ⬆️
... and 116 more

... and 448 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: Kevin Su <pingsutw@apache.org>
@pingsutw pingsutw changed the title More logs in databricks plugin Fix databricks plugin Oct 16, 2023
@pingsutw pingsutw marked this pull request as ready for review October 16, 2023 21:46
Copy link

@givanovexpe givanovexpe left a comment

Choose a reason for hiding this comment

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

This fixes the problems we've seen when testing the plugin.

@pingsutw pingsutw merged commit 481a966 into master Oct 17, 2023
37 of 38 checks passed
@pingsutw pingsutw deleted the databricks-bug branch October 17, 2023 22:25
squiishyy pushed a commit to squiishyy/flyte that referenced this pull request Oct 18, 2023
---------

Signed-off-by: Kevin Su <pingsutw@apache.org>
Signed-off-by: squiishyy <joe@union.ai>
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.

3 participants