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

No longer using GAX batching for WriteLogEntries #632

Closed
jamestep opened this issue Aug 19, 2021 · 7 comments
Closed

No longer using GAX batching for WriteLogEntries #632

jamestep opened this issue Aug 19, 2021 · 7 comments
Assignees
Labels
api: logging Issues related to the googleapis/java-logging API. lang: java Issues specific to Java. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@jamestep
Copy link

After some investigation, it seems like since version 2.1.4 of this library, WriteLogEntries is no longer using GAX batching.

This is the specific line that's an issue, which changed in this commit. It switched from createBatchingCallable to createUnaryCallable, despite keeping the BatchingCallSettings logic.

I understand the issue may not be in this library, but I'm not familiar with microgenerator / gapic / autosynth / synthtool and this seemed like a good place to start.

This issue means we are unable to update beyond 2.1.3 as we hit the GCP WriteLogEntries request quota very quickly.

@product-auto-label product-auto-label bot added the api: logging Issues related to the googleapis/java-logging API. label Aug 19, 2021
@jamestep
Copy link
Author

Some important additional info: Here is the gapic yaml with the batching definition for WriteLogEntries from googleapis. It does seem to have moved around / between files over time.

@minherz
Copy link
Contributor

minherz commented Sep 1, 2021

@Neenu1995 can you have a look into this issue, please.

@minherz minherz added lang: java Issues specific to Java. type: question Request for information or clarification. Not an issue. labels Sep 1, 2021
@minherz
Copy link
Contributor

minherz commented Sep 1, 2021

@jamestep we have had similar discussion in googleapis/java-logging-logback#134. Do you have additional info showing that batching does not work?

@jamestep
Copy link
Author

Yes I do have some additional info:

  • For our project, hitting the GCP "Log ingestion requests per minute" with the only change being the version of google-cloud-logging
  • This line in 2.1.3 in GrpcLoggingServiceV2Stub which changed to this line in 2.1.4.
  • As indicated by others, there are BatchingCallSettings for writeLogEntries. But from the above code change it is clear that writeLogEntriesCallable switched from being created with createBatchingCallable to being created with createUnaryCallable. createUnaryCallable will not perform any batching, even if provided with batching settings.
  • The debugging screenshots below

See below when debugging, writeLogEntries is created with createBatchingCallable in 2.1.3:
cloud-logging-2 1 3-create-batching-callable
And then BatchingCallable / BatchedFuture is used:
cloud-logging-2 1 3-batched-future

But then debugging 2.1.4, writeLogEntries is created with createUnaryCallable:
cloud-logging-2 1 4-create-unary-callable
And then BatchingCallable / BatchedFuture is never used:
cloud-logging-2 1 4-batched-future-not-called

@minherz
Copy link
Contributor

minherz commented Oct 3, 2021

Thank you for this info. We need to check the problem with auto-generated code and will update you about next steps soon.
Please mind that the currently supported version of the library is 3.2.0. We will run the tests to see batching behavior on this version.

@minherz minherz added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed type: question Request for information or clarification. Not an issue. labels Oct 3, 2021
minherz added a commit that referenced this issue Oct 4, 2021
Changes GrpcLoggingServiceV2Stub stub to make it createBatchingCallable.
Modify WriteLogEntry sample to demo batching is working.

See #632 for problem description
@minherz
Copy link
Contributor

minherz commented Oct 5, 2021

Thank you for your patience. We've confirmed the problem. Hopefully, it will be addressed in the next release. The release time will be dependent on the gapic generator fix.

@minherz
Copy link
Contributor

minherz commented Oct 26, 2021

Fixed in #726

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the googleapis/java-logging API. lang: java Issues specific to Java. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

3 participants