feat: Query.count() implementation, part 1 #1026
Conversation
dconeybe
added
the
api: firestore
|
🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use -- conventional-commit-lint bot |
dconeybe
force-pushed
the
dconeybe/CountDraft1
branch
from
0dfda07 to
7467804
Compare
dconeybe
changed the title
dconeybe
force-pushed
the
dconeybe/CountDraft1
branch
from
7467804 to
1be2496
Compare
google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITAggregateQueryTest.java
Show resolved
Hide resolved
| throws Exception { | ||
| CollectionReference collection = createCollectionWithDocuments(20); | ||
| ApiFuture<AggregateQuerySnapshot> task = collection.count().get(); | ||
| collection.getFirestore().close(); |
There was a problem hiding this comment.
Interesting, will this be flaky?
There was a problem hiding this comment.
close() appears to peform a "graceful" shutdown, in which in-flight operations are allowed to run to completion. That being the case, this shouldn't be flaky.
There was a problem hiding this comment.
So if for whatever reason the backend does not respond, close() will hang forever?
There was a problem hiding this comment.
I believe so, yes. The main thing I want to test is that nothing "bad" happens, like silly exceptions that simply didn't take this code path into account. I can confirm if you'd like.
There was a problem hiding this comment.
Well, I would like to see it for my curiosity now. Not a blocker for this PR though.
There was a problem hiding this comment.
I looked into whether or not close() will hang forever if the server takes too long to respond, and, it appears that no, it will not hang. That being said, it's difficult to tell from stepping through the gory details of grpc's shutdown logic.
google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryCountTest.java
Outdated
Show resolved
Hide resolved
google-cloud-firestore/src/test/java/com/google/cloud/firestore/it/ITQueryCountTest.java
Outdated
Show resolved
Hide resolved
| * Blocks the calling thread until the given future completes. Note that this method does not | ||
| * check the success or failure of the future; it returns regardless of its success or failure. | ||
| */ | ||
| private static void await(ApiFuture<?> future) throws InterruptedException { |
There was a problem hiding this comment.
future.get() will throw the exception if the task fails, which isn't the desired behavior for the singular use of this function.
There was a problem hiding this comment.
Will this do:
try {future.get()} finally {assertTrue(future.done());}
?
There was a problem hiding this comment.
That would still throw the exception thrown by get(). The caller of await() just wants to make sure that the future completes, but doesn't care about its success or failure.
…ta between consecutive read times
| throws Exception { | ||
| CollectionReference collection = createCollectionWithDocuments(20); | ||
| ApiFuture<AggregateQuerySnapshot> task = collection.count().get(); | ||
| collection.getFirestore().close(); |
There was a problem hiding this comment.
Well, I would like to see it for my curiosity now. Not a blocker for this PR though.
2 participants
Initial implementation of server-side query result set counting.
This initial implementation is still missing some work, which will be done in follow-up PRs:
Transaction.getCount())Tracing.getTracer())