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: add manual layer for v1 endpoint #16

Merged
merged 7 commits into from Mar 3, 2020

Conversation

shollyman
Copy link
Contributor

@shollyman shollyman commented Feb 29, 2020

  • adds a per-endpoint system session in nox
  • migrate the manual client/reader work for v1
  • TableReferences went away as a first class message, now just a
    formatted string
  • changes to read rows
    • estimated_row_count removed due to differences in reported status
    • you no longer have to deal with a StreamPosition message, instead
      there's just a stream name and an offset as top-level request fields
  • session creation changes
    • you now provide a prototypical ReadSession message when requesting
      a new read session, and most options (like selected fields, table
      modifiers, and data format) have moved inside it.
    • requested_streams -> max_stream_count

There's additional changes to the surface, but there wasn't much manual
help in front of it.

* adds a per-endpoint system session in nox
* migrate the manual client/reader work for v1
* TableReferences went away as a first class message, now just a
  formatted string
* changes to read rows
  * estimated_row_count removed due to differences in reported status
  * you no longer have to deal with a StreamPosition message, instead
    there's just a stream name and an offset as top-level request fields
* session creation changes
  * you now provide a prototypical ReadSession message when requesting
    a new read session, and most options (like selected fields, table
    modifiers, and data format) have moved inside it.
  * requested_streams -> max_stream_count

There's additional changes to the surface, but there wasn't much manual
help in front of it.
@googlebot googlebot added the cla: yes label Feb 29, 2020
@shollyman shollyman requested review from tswast and busunkim96 Feb 29, 2020
@shollyman
Copy link
Contributor Author

@shollyman shollyman commented Feb 29, 2020

failure related to coverage. will poke at that next.

tswast
tswast approved these changes Mar 3, 2020
Copy link
Contributor

@tswast tswast left a comment

LGTM once that docstring is updated.

google/cloud/bigquery_storage_v1/reader.py Show resolved Hide resolved
google/cloud/bigquery_storage_v1/reader.py Show resolved Hide resolved
noxfile.py Outdated
"""Run the system test suite."""
system_test_path = os.path.join("tests", "system.py")
system_test_folder_path = os.path.join("tests", "system")
def system_v1beta1(session):
Copy link
Collaborator

@busunkim96 busunkim96 Mar 3, 2020

Choose a reason for hiding this comment

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

Is there a specific reason for splitting up the system tests into two sessions?

If this needs to be made permanent noxfile.py should be added to the list of excludes in synth.py.

Copy link
Contributor Author

@shollyman shollyman Mar 3, 2020

Choose a reason for hiding this comment

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

I mostly did it to speed up the testing cycle, as it takes a couple minutes to get through system tests for a single endpoint. I can drop it now that I'm done, but is this common enough we should consider parameterizing or plumbing some form of nox flag?

Copy link
Collaborator

@busunkim96 busunkim96 Mar 3, 2020

Choose a reason for hiding this comment

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

Ah I see. Would you mind opening an issue on https://github.com/googleapis/synthtool/issues to discuss additional parametrizing in the noxfile?

Copy link
Contributor Author

@shollyman shollyman Mar 3, 2020

Choose a reason for hiding this comment

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

@shollyman shollyman merged commit a0fc0af into googleapis:master Mar 3, 2020
3 checks passed
@shollyman shollyman deleted the manuallayer branch Mar 3, 2020

@pytest.fixture(scope="session")
def local_shakespeare_table_reference(project_id):
return _TABLE_FORMAT.format(project_id, "public_samples_copy", "shakespeare")
Copy link
Contributor

@tswast tswast Oct 16, 2020

Choose a reason for hiding this comment

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

@shollyman I'm curious why the local copy of bigquery-public-data.samples.shakespeare? I'm adding a script to create this table in #74

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants