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 CollectionGroup#getPartitions(long) #478

Merged
merged 3 commits into from Jan 15, 2021
Merged

feat: add CollectionGroup#getPartitions(long) #478

merged 3 commits into from Jan 15, 2021

Conversation

BenWhitehead
Copy link
Collaborator

Add new method to CollectionGroup allowing getPartitions to return a Future<List<QueryPartition>>. This new method behaves similar to the CollectionGroup#getPartitions(long, ApiStreamObserver<QueryPartition>) providing a Future centric api.

@BenWhitehead BenWhitehead requested a review from a team as a code owner November 25, 2020 20:59
@BenWhitehead BenWhitehead requested a review from a team November 25, 2020 20:59
@product-auto-label product-auto-label bot added the api: firestore Issues related to the googleapis/java-firestore API. label Nov 25, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Nov 25, 2020
@codecov
Copy link

codecov bot commented Nov 25, 2020

Codecov Report

Merging #478 (a65f94f) into master (23d5415) will decrease coverage by 0.33%.
The diff coverage is 2.17%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #478      +/-   ##
============================================
- Coverage     74.43%   74.09%   -0.34%     
  Complexity     1108     1108              
============================================
  Files            66       66              
  Lines          5855     5883      +28     
  Branches        726      725       -1     
============================================
+ Hits           4358     4359       +1     
- Misses         1269     1296      +27     
  Partials        228      228              
Impacted Files Coverage Δ Complexity Δ
...va/com/google/cloud/firestore/CollectionGroup.java 12.30% <2.17%> (-6.62%) 1.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23d5415...a65f94f. Read the comment docs.

@BenWhitehead
Copy link
Collaborator Author

With the code that is in the PR right now, the future transform takes place on one of the gax threads leaving the invoking thread unblocked as can be seen in the following log snippet.

2021-01-07T21:38:31.080 [main] com.google.cloud.firestore.CollectionGroup - getPartitions(3)...
2021-01-07T21:38:31.098 [main] com.google.cloud.firestore.CollectionGroup - getPartitions(3) complete
2021-01-07T21:38:31.098 [main] com.google.cloud.firestore.CollectionGroup - calling future.get()...
2021-01-07T21:38:31.255 [Gax-4] com.google.cloud.firestore.CollectionGroup - transform->apply()
2021-01-07T21:38:31.260 [main] com.google.cloud.firestore.CollectionGroup - calling future.get() complete

If I change the implementation to adapt the existing ApiStreamObserver method like so

  public ApiFuture<List<QueryPartition>> getPartitionss(long desiredPartitionCount) {
    final SettableApiFuture<List<QueryPartition>> future = SettableApiFuture.create();
    getPartitions(desiredPartitionCount, new ApiStreamObserver<QueryPartition>() {
      final List<QueryPartition> accumulator = new ArrayList<>();
      @Override
      public void onNext(QueryPartition value) {
        accumulator.add(value);
      }

      @Override
      public void onError(Throwable t) {
        future.setException(t);
      }

      @Override
      public void onCompleted() {
        future.set(accumulator);
      }
    });
    return future;
  }

The invocations on the observer are done from the invoking thread meaning the future isn't really effective.

2021-01-07T21:38:50.196 [main] com.google.cloud.firestore.CollectionGroup - getPartitions(3)...
2021-01-07T21:38:50.301 [main] com.google.cloud.firestore.CollectionGroup - CollectionGroup.onNext
2021-01-07T21:38:50.303 [main] com.google.cloud.firestore.CollectionGroup - CollectionGroup.onNext
2021-01-07T21:38:50.303 [main] com.google.cloud.firestore.CollectionGroup - CollectionGroup.onCompleted
2021-01-07T21:38:50.303 [main] com.google.cloud.firestore.CollectionGroup - getPartitions(3) complete
2021-01-07T21:38:50.304 [main] com.google.cloud.firestore.CollectionGroup - calling future.get()...
2021-01-07T21:38:50.304 [main] com.google.cloud.firestore.CollectionGroup - calling future.get() complete

I thought about using rpcContext.getClient().getExecutor().submit(...) to wrap the call to getPartitions but that executor is the grpc-transport threadpool which we definitely don't want to block.

2021-01-07T21:44:20.290 [main] com.google.cloud.firestore.CollectionGroup - getPartitions(3)...
2021-01-07T21:44:20.292 [main] com.google.cloud.firestore.CollectionGroup - getPartitions(3) complete
2021-01-07T21:44:20.293 [main] com.google.cloud.firestore.CollectionGroup - calling future.get()...
2021-01-07T21:44:20.453 [grpc-transport-0] com.google.cloud.firestore.CollectionGroup - CollectionGroup.onNext
2021-01-07T21:44:20.455 [grpc-transport-0] com.google.cloud.firestore.CollectionGroup - CollectionGroup.onNext
2021-01-07T21:44:20.455 [grpc-transport-0] com.google.cloud.firestore.CollectionGroup - CollectionGroup.onCompleted
2021-01-07T21:44:20.455 [main] com.google.cloud.firestore.CollectionGroup - calling future.get() complete

@@ -101,4 +109,60 @@ public void getPartitions(

observer.onCompleted();
}

public ApiFuture<List<QueryPartition>> getPartitions(long desiredPartitionCount) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if you we could do something like this:

public Iterable<CollectionReference> listCollections() {
and expose an Iterable of QueryPartitions? I assume this won't work because of some threading issues, but it would be nice to re-use an existing pattern.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we go this route we lose the ability to chain the result of this method into a series of queries with the following pattern:

    ApiFuture<List<QuerySnapshot>> allQueries = ApiFutures.transformAsync(
        firestore.collectionGroup(randomColl.getId()).getPartitions(3),
        new ApiAsyncFunction<List<QueryPartition>, List<QuerySnapshot>>() {
          @Override
          public ApiFuture<List<QuerySnapshot>> apply(List<QueryPartition> input) {
            logit("ApiAsyncFunction.apply");
            List<ApiFuture<QuerySnapshot>> futures = new ArrayList<>();
            for (QueryPartition part : input) {
              logit("part = %s", part);
              futures.add(part.createQuery().get());
            }
            return ApiFutures.allAsList(futures);
          }
        },
        MoreExecutors.directExecutor()
    );
    List<QuerySnapshot> querySnapshots = allQueries.get();
2021-01-08T23:52:57.989  [main] com.google.cloud.firestore.CollectionGroup - transformAsync...
2021-01-08T23:52:58.008  [main] com.google.cloud.firestore.CollectionGroup - transformAsync Complete
2021-01-08T23:52:58.008  [main] com.google.cloud.firestore.CollectionGroup - allQueries.get()...
2021-01-08T23:52:58.111  [Gax-4] com.google.cloud.firestore.CollectionGroup - transform->apply()
2021-01-08T23:52:58.116  [Gax-4] com.google.cloud.firestore.CollectionGroup - ApiAsyncFunction.apply
2021-01-08T23:52:58.117  [Gax-4] com.google.cloud.firestore.CollectionGroup - part = com.google.cloud.firestore.QueryPartition@a9b0a06e
2021-01-08T23:52:58.131  [Gax-4] com.google.cloud.firestore.CollectionGroup - part = com.google.cloud.firestore.QueryPartition@19a03734
2021-01-08T23:52:58.309  [main] com.google.cloud.firestore.CollectionGroup - allQueries.get() complete

There is also additional state that has to be tracked because the iterator we return would have to potentially run past the end of the iterator returned by response.iterateAll() not to mention the iteration state now has to be split across the hasNext and next methods. (It's possible but actually much more code, so I can swap it in if you want me to).

@google-cla
Copy link

google-cla bot commented Jan 15, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to determine that you authored the commits in this PR. Maybe you used a different email address in the git commits than was used to sign the CLA? If someone else authored these commits, then please add them to this pull request and have them confirm that they're okay with them being contributed to Google. If there are co-authors, make sure they're formatted properly.

In order to pass this check, please resolve this problem and then comment@googlebot I fixed it... If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: no This human has *not* signed the Contributor License Agreement. cla: yes This human has signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. cla: no This human has *not* signed the Contributor License Agreement. labels Jan 15, 2021
Add new method to CollectionGroup allowing getPartitions to return a
Future<List<QueryPartition>>. This new method behaves similar to the
CollectionGroup#getPartitions(long, ApiStreamObserver<QueryPartition>)
providing a Future centric api.
@BenWhitehead BenWhitehead merged commit bab064e into googleapis:master Jan 15, 2021
@BenWhitehead BenWhitehead deleted the future-partition-query branch January 15, 2021 19:48
gcf-merge-on-green bot pushed a commit that referenced this pull request Jan 20, 2021
🤖 I have created a release \*beep\* \*boop\* 
---
## [2.2.0](https://www.github.com/googleapis/java-firestore/compare/v2.1.0...v2.2.0) (2021-01-20)


### Features

* Add bundle proto building ([#271](https://www.github.com/googleapis/java-firestore/issues/271)) ([994835c](https://www.github.com/googleapis/java-firestore/commit/994835c0a3be077404afa60abd4d4685d17fb533))
* add bundle.proto from googleapis/googleapis ([#407](https://www.github.com/googleapis/java-firestore/issues/407)) ([37da386](https://www.github.com/googleapis/java-firestore/commit/37da386875d1b65121e8a9a92b1a000537f07625))
* add CollectionGroup#getPartitions(long) ([#478](https://www.github.com/googleapis/java-firestore/issues/478)) ([bab064e](https://www.github.com/googleapis/java-firestore/commit/bab064edde26325bf0041ffe28d4c63b97a089c5))
* add implicit ordering for startAt(DocumentReference) calls ([#417](https://www.github.com/googleapis/java-firestore/issues/417)) ([aae6dc9](https://www.github.com/googleapis/java-firestore/commit/aae6dc960f7c42830ceed23c65acaad3e457dcff))
* add max/min throttling options to BulkWriterOptions ([#400](https://www.github.com/googleapis/java-firestore/issues/400)) ([27a9397](https://www.github.com/googleapis/java-firestore/commit/27a9397f67e151d723241c80ccb2ec9f1bfbba1c))
* add success and error callbacks to BulkWriter ([#483](https://www.github.com/googleapis/java-firestore/issues/483)) ([3c05037](https://www.github.com/googleapis/java-firestore/commit/3c05037e8fce8d3ce4907fde85bd254fc98ea588))
* Implementation of Firestore Bundle Builder ([#293](https://www.github.com/googleapis/java-firestore/issues/293)) ([fd5ef90](https://www.github.com/googleapis/java-firestore/commit/fd5ef90b6681cc67aeee6c95f3de80267798dcd0))
* Release bundles ([#466](https://www.github.com/googleapis/java-firestore/issues/466)) ([3af065e](https://www.github.com/googleapis/java-firestore/commit/3af065e32b193931c805b576f410ad90124b43a7))


### Bug Fixes

* add @BetaApi, make BulkWriter public, and refactor Executor ([#497](https://www.github.com/googleapis/java-firestore/issues/497)) ([27ff9f6](https://www.github.com/googleapis/java-firestore/commit/27ff9f6dfa92cac9119d2014c24a0759baa44fb7))
* **build:** sample checkstyle violations ([#457](https://www.github.com/googleapis/java-firestore/issues/457)) ([777ecab](https://www.github.com/googleapis/java-firestore/commit/777ecabd1ce12cbc5f4169de6c23a90f98deac06))
* bulkWriter: writing to the same doc doesn't create a new batch ([#394](https://www.github.com/googleapis/java-firestore/issues/394)) ([259ece8](https://www.github.com/googleapis/java-firestore/commit/259ece8511db71ea79cc1a080eb785a15db88756))
* empty commit to trigger release-please ([fcef0d3](https://www.github.com/googleapis/java-firestore/commit/fcef0d302cd0a9339d82db73152289d6f9f67ff2))
* make BulkWriterOptions public ([#502](https://www.github.com/googleapis/java-firestore/issues/502)) ([6ea05be](https://www.github.com/googleapis/java-firestore/commit/6ea05beb3f27337bef910ca64f0e3f32de6b7e98))
* retry Query streams ([#426](https://www.github.com/googleapis/java-firestore/issues/426)) ([3513cd3](https://www.github.com/googleapis/java-firestore/commit/3513cd39ff43d26c8432c05ce20693350539ae8f))
* retry transactions that fail with expired transaction IDs ([#447](https://www.github.com/googleapis/java-firestore/issues/447)) ([5905438](https://www.github.com/googleapis/java-firestore/commit/5905438af6501353e978210808834a26947aae95))
* verify partition count before invoking GetPartition RPC ([#418](https://www.github.com/googleapis/java-firestore/issues/418)) ([2054ae9](https://www.github.com/googleapis/java-firestore/commit/2054ae971083277e1cf81c2b57500c40a6faa0ef))


### Documentation

* **sample:** normalize firestore sample's region tags ([#453](https://www.github.com/googleapis/java-firestore/issues/453)) ([b529245](https://www.github.com/googleapis/java-firestore/commit/b529245c75f770e1b47ca5d9561bab55a7610677))


### Dependencies

* remove explicit version for jackson ([#479](https://www.github.com/googleapis/java-firestore/issues/479)) ([e2aecfe](https://www.github.com/googleapis/java-firestore/commit/e2aecfec51465b8fb3413337a76f9a3de57b8500))
* update dependency com.google.cloud:google-cloud-conformance-tests to v0.0.12 ([#367](https://www.github.com/googleapis/java-firestore/issues/367)) ([2bdd846](https://www.github.com/googleapis/java-firestore/commit/2bdd84693bbd968cafabd0e7ee56d1a9a7dc31ca))
* update dependency com.google.cloud:google-cloud-conformance-tests to v0.0.13 ([#411](https://www.github.com/googleapis/java-firestore/issues/411)) ([e6157b5](https://www.github.com/googleapis/java-firestore/commit/e6157b5cb532e0184125355b12115058e72afa67))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.10.0 ([#383](https://www.github.com/googleapis/java-firestore/issues/383)) ([cb39ee8](https://www.github.com/googleapis/java-firestore/commit/cb39ee820c2f67e22da623f5a6eaa7ee6bf351e2))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.10.2 ([#403](https://www.github.com/googleapis/java-firestore/issues/403)) ([991dd81](https://www.github.com/googleapis/java-firestore/commit/991dd810360e654fca0b53e0611da0cd77febc7c))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.12.1 ([#425](https://www.github.com/googleapis/java-firestore/issues/425)) ([b897ffa](https://www.github.com/googleapis/java-firestore/commit/b897ffa90427a8f597c02c24f80d1d162be48b23))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.13.0 ([#430](https://www.github.com/googleapis/java-firestore/issues/430)) ([0f8f218](https://www.github.com/googleapis/java-firestore/commit/0f8f218678c3ddebb73748c382cab8e38c2f140d))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.14.1 ([#446](https://www.github.com/googleapis/java-firestore/issues/446)) ([e241f8e](https://www.github.com/googleapis/java-firestore/commit/e241f8ebbfdf202f00424177c69962311b37fc88))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.15.0 ([#460](https://www.github.com/googleapis/java-firestore/issues/460)) ([b82fc35](https://www.github.com/googleapis/java-firestore/commit/b82fc3561d1a397438829ab69df24141481369a2))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.16.0 ([#481](https://www.github.com/googleapis/java-firestore/issues/481)) ([ae98824](https://www.github.com/googleapis/java-firestore/commit/ae988245e6d6391c85414e9b6f7ae1b8148c3a6d))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.16.1 ([4ace93c](https://www.github.com/googleapis/java-firestore/commit/4ace93c7be580a8f7870e71cad2dc19bb5fdef29))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.17.0 ([#487](https://www.github.com/googleapis/java-firestore/issues/487)) ([e11e472](https://www.github.com/googleapis/java-firestore/commit/e11e4723bc75727086bee0436492f458def29ff5))
* update dependency com.google.cloud:google-cloud-shared-dependencies to v0.18.0 ([#495](https://www.github.com/googleapis/java-firestore/issues/495)) ([f78720a](https://www.github.com/googleapis/java-firestore/commit/f78720a155f1294321f05266b9a546bbf2cb9a04))
* update jackson dependencies to v2.11.3 ([#396](https://www.github.com/googleapis/java-firestore/issues/396)) ([2e176e2](https://www.github.com/googleapis/java-firestore/commit/2e176e2f864262f31e6f93705fa7e794023b9649))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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 googleapis/java-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

3 participants