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

State that Datastore IO is not supported in Streaming scenario. #7758

Merged
merged 1 commit into from
Feb 7, 2019

Conversation

Ardagan
Copy link
Contributor

@Ardagan Ardagan commented Feb 6, 2019

Users try to utilize Datastore IO with Python Streaming that is not supported.
Explicitly state in documentation that this scenario is not supported.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status
Build Status
Build Status
Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@Ardagan
Copy link
Contributor Author

Ardagan commented Feb 6, 2019

R: @aaltay

@Ardagan Ardagan changed the title [DoNotMerge] State that Datastore IO is not supported in Streaming scenario. State that Datastore IO is not supported in Streaming scenario. Feb 6, 2019
@@ -53,6 +53,9 @@

class ReadFromDatastore(PTransform):
"""A ``PTransform`` for reading from Google Cloud Datastore.

Copy link
Member

Choose a reason for hiding this comment

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

We can drop this change. Web site changes are sufficient. The reasons this notice is not consistent, there are other IOs that will not work in streaming mode and they do not have comments. It is also likely to not get updated as the support is added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I still feel that we should add such information to code base as well.
But concerned of two independent sources of truth that can go out of sync.

Copy link
Member

@aaltay aaltay left a comment

Choose a reason for hiding this comment

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

Thank you

@aaltay aaltay merged commit 08f7c6d into apache:master Feb 7, 2019
@Ardagan Ardagan deleted the DatastoreIODocUpdate branch February 7, 2019 21:06
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
* Initial plumbing for collection group queries

* Don't assume direct children for collection group queries.

* trim document path to DocumentReference

* add unit tests

* ensure all_descendants is set after calling other query methods

* port test for node impl

* port tests from node impl

* Fix collection group test on Python 2.7.

Blacken.

* Use '_all_descendants' in 'Query.__eq__'.

* Ensure '_all_descendants' propagates when narrowing query.

* Refactor collection group system tests.

Skip the one for 'where', because it requires a custom index.

* Match node test's collection group ID / expected output.

See:
https://github.com/googleapis/nodejs-firestore/pull/578/files#diff-6b8febc8d51ea01205628091b3611eacR1188

* Match Node test for filter on '__name__'.

Note that this still doesn't pass, so remains skipped.

* Blacken.

* Fix / unskip systest for collection groups w/ filter on '__name__'.

* Blacken

* 100% coverage.

* Lint
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.

2 participants