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

Add serverless_compute_id field to the config #675

Closed
wants to merge 5 commits into from

Conversation

alexkh-db
Copy link
Contributor

@alexkh-db alexkh-db commented Jun 11, 2024

Changes

We propose adding a new serverless_compute_id attribute in the config that will be used to enable serverless compute in the downstream applications. The default value to enable serverless is serverless_compute_id = auto, other values might be used in the future if needed. Config doesn't have to validate the value in serverless_compute_id, this will be done in the downstream applications.

Tests

  • make test run locally
  • make fmt applied
  • relevant integration tests applied

@codecov-commenter
Copy link

codecov-commenter commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.69%. Comparing base (f50af60) to head (d135f04).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #675   +/-   ##
=======================================
  Coverage   57.69%   57.69%           
=======================================
  Files          48       48           
  Lines       33079    33080    +1     
=======================================
+ Hits        19084    19085    +1     
  Misses      13995    13995           

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

Copy link

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

LGTM. Docs?

@alexkh-db
Copy link
Contributor Author

LGTM. Docs?

I couldn't find any docs for the other related fields, e.g. cluster_id, so i think we are not documenting it (at least yet). @mgyucht please lmk if documentation is needed

@grundprinzip
Copy link

Config doesn't have to validate the value in serverless_gc_id, this will be done in the downstream applications.

Isn't the only allowed value at the moment "auto" then? If this is the case, shouldn't it have validation?

@alexkh-db
Copy link
Contributor Author

Config doesn't have to validate the value in serverless_gc_id, this will be done in the downstream applications.

Isn't the only allowed value at the moment "auto" then? If this is the case, shouldn't it have validation?

i'm open to both options, the pros for not validating in config IMO are:

  1. config is doing little validation for other fields and mostly leaves validation to the downstream applications. not sure if we should single out serverless_gc_id field
  2. if dbconnect or any other downstream applications will need to add another value except auto, databricks-sdk code doesn't need to change and the user has one less package to upgrade

@grundprinzip
Copy link

Sorry for being horrible annoying. According to https://docs.databricks.com/en/compute/serverless.html I would just name the field serverless_compute_id.

@alexkh-db alexkh-db changed the title adding serverless_gc_id field to the Config adding serverless_compute_id field to the Config Jun 14, 2024
@alexkh-db alexkh-db requested a review from pietern June 19, 2024 11:27
@pietern pietern changed the title adding serverless_compute_id field to the Config Add serverless_compute_id field to the config Jun 19, 2024
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.

5 participants