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

feat: expose reservation usage stats on jobs #524

Merged
merged 3 commits into from Feb 17, 2021
Merged

Conversation

@plamut
Copy link
Contributor

@plamut plamut commented Feb 16, 2021

Closes #507.

This PR exposes reservation usage stats on job objects.

PR checklist:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)
@plamut plamut requested a review from as a code owner Feb 16, 2021
@plamut plamut requested review from stephaniewang526 and removed request for Feb 16, 2021
@google-cla google-cla bot added the cla: yes label Feb 16, 2021
Copy link
Contributor

@tswast tswast left a comment

We need to add to https://github.com/googleapis/python-bigquery/blob/master/docs/reference.rst under "job-related types" too.

I know in most libraries we use automodule, but it probably makes sense to continue to have a central index for BigQuery, since we have a lot more modules than most.

Loading

@plamut plamut requested a review from tswast Feb 17, 2021
tswast
tswast approved these changes Feb 17, 2021
@@ -73,6 +74,16 @@ def _error_result_to_exception(error_result):
)


ReservationUsage = namedtuple("ReservationUsage", "name slot_ms")
Copy link
Contributor

@tswast tswast Feb 17, 2021

Choose a reason for hiding this comment

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

Interesting!

This is a little different from our usual "class with _properties dictionary" approach, but does seem to save us some boilerplate. I'll allow it. :-)

Do you think we should do this for our other read-only classes?

Loading

Copy link
Contributor Author

@plamut plamut Feb 17, 2021

Choose a reason for hiding this comment

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

I though a namedtuple would better communicate that it's a read-only (output-only) property, although our "_properties classes" can also have read-only properties - but yeah, a namedtuple requires much less boilerplate.

(FWIW, I personally view these output-only classes merely as named read-only containers with descriptive attribute names, and using a class with a bunch of read-only properties is just an implementation detail)

We can eventually refactor other output-only classes to namedtuples for consistency, yes, but I wouldn't do it just yet. There's not that much immediate value in it, as that code has already been written, and there might be some legit use cases out there that we don't know about... let's first wait and see how a namedtuple performs when released into the wild.

Loading

google/cloud/bigquery/job/base.py Outdated Show resolved Hide resolved
Loading
@plamut plamut merged commit 4ffb4e0 into googleapis:master Feb 17, 2021
10 of 11 checks passed
Loading
@plamut plamut deleted the iss-507 branch Feb 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants