fix: abstract batch resource and add method to determine if batch should be flushed#1790
fix: abstract batch resource and add method to determine if batch should be flushed#1790blakeli0 merged 3 commits intogoogleapis:mainfrom
Conversation
mutianf
changed the title
igorbernstein2
left a comment
igorbernstein2
left a comment
There was a problem hiding this comment.
lgtm after comments are addressed
gax-java/gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/batching/DefaultBatchResource.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/batching/DefaultBatchResource.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/batching/DefaultBatchResource.java
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/batching/DefaultBatchResource.java
Outdated
Show resolved
Hide resolved
| long countBytes(ElementT element); | ||
|
|
||
| /** Creates a new {@link BatchResource} with ElementT. */ | ||
| default BatchResource createResource(ElementT element) { |
There was a problem hiding this comment.
Do we need this helper method in BatchingDescriptor? I would prefer to call DefaultBatchResource.create(1, countBytes(element)) directly.
There was a problem hiding this comment.
Yes, in bigtable client we want to override the BatchResource implementation, so we don't want to create BatchResource directly in the BatcherImpl code.
| } | ||
|
|
||
| /** Create an empty {@link BatchResource}. */ | ||
| default BatchResource createEmptyResource() { |
There was a problem hiding this comment.
Same comment for this method.
gax-java/gax/src/main/java/com/google/api/gax/batching/DefaultBatchResource.java
Outdated
Show resolved
Hide resolved
| * batch needs to be flushed. | ||
| */ | ||
| @InternalApi("For google-cloud-java client use only.") | ||
| public interface BatchResource { |
There was a problem hiding this comment.
Do we have to create an interface for it? The default implementation looks like a POJO to me, I would prefer not to have a separate interface if we don't plan to have multiple implementations.
There was a problem hiding this comment.
bigtable client will need to override it and have a different implementation :(
gax-java/gax/src/main/java/com/google/api/gax/batching/BatchingDescriptor.java
Outdated
Show resolved
Hide resolved
gax-java/gax/src/main/java/com/google/api/gax/batching/BatcherImpl.java
Outdated
Show resolved
Hide resolved
|
[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed! |
|
[java_showcase_integration_tests] SonarCloud Quality Gate failed.
|
|
[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed! |
blakeli0
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
blakeli0
added
the
automerge
gcf-merge-on-green
bot
removed
the
automerge
3 participants
Abstract the element and byte counting to a
BatchResourceobject so client libraries can add other dimensions to it. Also movedshouldBeFulllogic toBatchResourceso libraries can override it and use the other dimensions to determine if the batch should be flushed.