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.

Sign up for GitHub

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

Jump to bottom

Fix LoggingHandler flush #1795

Closed
pongad opened this issue Mar 23, 2017 · 6 comments
Closed

Fix LoggingHandler flush #1795

pongad opened this issue Mar 23, 2017 · 6 comments
Assignees
@garrettjonesgoogle
Labels
api: logging Issues related to the Cloud Logging API. 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

@pongad
Copy link
Contributor

pongad commented Mar 23, 2017 •
edited
Loading

Logging handler's flush() calls LoggingImpl.write which eventually calls LoggingClient.writeLogEntries which uses bundling. So, handler flushing just moves messages from one buffer to another without actually making any RPC call.

Proposed fix

LoggingHandler should keep a count of pending messages. The count increments with calls to publish and decrements when RPC returns.

Flush should force batcher to issue all buffered messages and just wait for the count to reach zero. No call to publish may succeed when flush is "in flight" otherwise flush could wait forever.

Encountering a log above the configured flush level should also force a flush.
@pongad pongad added api: logging Issues related to the Cloud Logging API. Status: Release Blocking labels Mar 23, 2017
@pongad pongad self-assigned this Mar 23, 2017
@pongad pongad mentioned this issue Mar 23, 2017
Merged
@garrettjonesgoogle
Copy link
Member

@pongad could you reword the description in terms of a command instead of a problem? (e.g. "Fix logging flush")

@pongad pongad changed the title Logging handler flush is broken Fix LoggingHandler flush Mar 23, 2017
@pongad
Copy link
Contributor Author

pongad commented Mar 23, 2017

@garrettjonesgoogle I never seem to be able to remember. Done.

@garrettjonesgoogle
Copy link
Member

Also reword the extended description, recommending a course of action to take.

pongad added a commit that referenced this issue Mar 24, 2017
@pongad
logging: remove handler's buffer (#1796)
b5f2281 
LoggingHandler's buffer is redundant,
since it's already using the batching feature.

LoggingHandler.flush previously just flushes its own buffer
and put messages in the batcher's buffer,
without necessarily making RPC calls.
This PR does not fix this problem,
but it makes flush obviously wrong instead of subtly.

The test for flush size is also removed.
Flush size should be forwarded to the batcher,
which already has its own test.

Updates #1795.
@garrettjonesgoogle
Copy link
Member

This properly waits for the RPCs to finish with @pongad 's last change; the triggering of RPCs by the call can be done after GA. Removing the release-blocking label.

@garrettjonesgoogle garrettjonesgoogle added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. and removed POST GA - P1 labels Apr 26, 2017
@garrettjonesgoogle garrettjonesgoogle added the type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. label Jul 18, 2017
@garrettjonesgoogle garrettjonesgoogle added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Aug 10, 2017
@pongad
Copy link
Contributor Author

pongad commented Dec 12, 2017

@garrettjonesgoogle I think I need to send this back to you. The generated client must expose a way to flush. I don't think this is possible at the moment.

@pongad pongad assigned garrettjonesgoogle and unassigned pongad Dec 12, 2017
@pongad pongad mentioned this issue Jan 22, 2018
Closed
@pongad
Copy link
Contributor Author

pongad commented Feb 1, 2018

I'll resolve this as duplicate of #2796.

@pongad pongad closed this as completed Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@garrettjonesgoogle garrettjonesgoogle

Labels
api: logging Issues related to the Cloud Logging API. 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
Milestone
No milestone
Development

No branches or pull requests
2 participants
@pongad @garrettjonesgoogle