[go: nahoru, domu]

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

Firestore: Treat 'None' as EOF in 'Watch.on_snapshot'. #8687

Merged
merged 5 commits into from
Jul 24, 2019
Merged

Firestore: Treat 'None' as EOF in 'Watch.on_snapshot'. #8687

merged 5 commits into from
Jul 24, 2019

Conversation

tseaver
Copy link
Contributor
@tseaver tseaver commented Jul 16, 2019

@tseaver tseaver added the api: firestore Issues related to the Firestore API. label Jul 16, 2019
@tseaver tseaver requested a review from crwilcox July 16, 2019 18:22
@tseaver tseaver requested a review from frankyn as a code owner July 16, 2019 18:22
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 16, 2019
Copy link
Contributor
@plamut plamut left a comment

Choose a reason for hiding this comment

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

LGTM (the code changes)

watch.py needs to be blackened, though, and I'm not sure about the API core failure (unrelated?)

Update: The API core failure was apparently flakiness.

@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 17, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 17, 2019
@plamut plamut removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 18, 2019
@plamut
Copy link
Contributor
plamut commented Jul 18, 2019

Something weird is going on, I removed and re-added the force run label, the label is there, but there is no record of that on the PR timeline, and the yoshi-kokoro bot does not see it.

@plamut plamut removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 18, 2019
@tseaver tseaver added kokoro:force-run Add this label to force Kokoro to re-run the tests. and removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 18, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 18, 2019
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 19, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 19, 2019
@crwilcox crwilcox added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 19, 2019
@crwilcox
Copy link
Contributor

I cleaned out firestore to see if that helps. forcing a ci run.

@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 19, 2019
@tseaver
Copy link
Contributor Author
tseaver commented Jul 19, 2019

The Kokoro jobs for Bigtable and Natural Language both fail due to a Kokoro-internal glitch:

Cannot check out non-mergeable pull request as if merged.

@plamut
Copy link
Contributor
plamut commented Jul 23, 2019

@tseaver Is there anything we can do to get around the glitch?

@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 23, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 23, 2019
@tseaver
Copy link
Contributor Author
tseaver commented Jul 23, 2019

@plamut

Is there anything we can do to get around the glitch?

I'm actually worried that there is something more sinister at play.

@crwilcox Can you double-check that the clean-out really worked in the CI project? E.g.:

documents = []
for collection in client.collections():
    documents.extend(collection.list_documents()
assert len(documents) == 0

@tseaver
Copy link
Contributor Author
tseaver commented Jul 23, 2019

I have added a commit to make the system test output verbose, so that we can figure out which test is hanging.

@tseaver
Copy link
Contributor Author
tseaver commented Jul 23, 2019

@crwilcox I'm expecting this Kokoro run to time out again: can you post the hung systest output when it does? Or maybe you have the keys to see the output even before it times out?

Enable debugging hung tests.
- Use a single unique resource ID across the entire systest run.
- Add missing document cleanups in collection_group tests.
- Avoid calling 'watch.unsubscribe' in cleanup:  it causes hangs,
  and there is no server-side resource leaked.
- Add a 'cleanup_firestore_documents.py' utility script.
@tseaver tseaver merged commit 1dc92c7 into googleapis:master Jul 24, 2019
@tseaver tseaver deleted the 8650-firestore-watch-handle-none-response branch July 24, 2019 01:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the Firestore API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants