Skip to content

Logging: call flush before close#4784

Merged
chingor13 merged 2 commits intogoogleapis:masterfrom
meltsufin:logging-flush-close
May 1, 2019
Merged

Logging: call flush before close#4784
chingor13 merged 2 commits intogoogleapis:masterfrom
meltsufin:logging-flush-close

Conversation

@meltsufin
Copy link
Copy Markdown
Member

@meltsufin meltsufin commented Mar 28, 2019

Also prevent more logs to be written after close.

Fixes #4783.

@meltsufin
Logging: call flush before close
4a2297f 
Also prevent more logs to be written after close.

Fixes googleapis#4783.
@meltsufin meltsufin requested a review from a team March 28, 2019 18:13
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 28, 2019
@sduskis
Copy link
Copy Markdown
Contributor

sduskis commented Mar 28, 2019

There was a similar PR in #3909. Right now, flush() ought to be called, and ideally we force the flushing to occur. Unfortunately, flush() just waits for a timer to complete. @chingor13 didn't want to merge the change for the first attempt at this bug fix. We need to figure out how to fix this for the long haul, once @chingor13 is back.

@sduskis sduskis added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 28, 2019
@meltsufin
Copy link
Copy Markdown
Member Author

meltsufin commented Mar 28, 2019

@sduskis The critical problem is that everything gets closed while there are pending API calls. Calling flush() before close() actually fixes the issue.
This is a critical issue for us in Spring Cloud GCP (#4784) as it currently hangs apps for 5 minutes on startup as GRPC is unable to complete those pending requests after the logging service was shut down.
Unless you think there is harm in this change, can we merge this, and @chingor13 can redo the fix when he comes back?

@sduskis
Copy link
Copy Markdown
Contributor

sduskis commented Mar 28, 2019 •
edited
Loading

@meltsufin, I understand why this is critical. Merging this is @chingor13's call. The fix will cause unexpected shutdown latency, which is why this fix wasn't merged the last time it was proposed.

@sduskis
Copy link
Copy Markdown
Contributor

sduskis commented Apr 2, 2019

@meltsufin, can you please run mvn com.coveo:fmt-maven-plugin:format to appease the linter?

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 3, 2019 •
edited
Loading

Codecov Report

Merging #4784 into master will increase coverage by 0.77%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4784      +/-   ##
============================================
+ Coverage     49.78%   50.55%   +0.77%     
- Complexity    21509    23235    +1726     
============================================
  Files          2168     2192      +24     
  Lines        212387   221037    +8650     
  Branches      24147    24290     +143     
============================================
+ Hits         105737   111747    +6010     
- Misses        99152   100895    +1743     
- Partials       7498     8395     +897
Impacted Files Coverage Δ Complexity Δ
...ain/java/com/google/cloud/logging/LoggingImpl.java 87.81% <0%> (-0.21%) 73 <1> (+12)
...oud/automl/v1beta1/stub/PredictionServiceStub.java 20% <0%> (-30%) 1% <0%> (ø)
...va/com/google/cloud/compute/v1/InstanceClient.java 55.67% <0%> (-7.76%) 127% <0%> (+31%)
...oogle/cloud/kms/v1/KeyManagementServiceClient.java 62.78% <0%> (-7.22%) 91% <0%> (+22%)
...ava/com/google/cloud/compute/v1/ProjectClient.java 57.07% <0%> (-6.99%) 55% <0%> (+13%)
...va/com/google/cloud/compute/v1/SnapshotClient.java 55.73% <0%> (-6.88%) 31% <0%> (+7%)
...a/com/google/cloud/compute/v1/NodeGroupClient.java 60.36% <0%> (-6.82%) 51% <0%> (+12%)
.../com/google/cloud/compute/v1/SubnetworkClient.java 60.36% <0%> (-6.82%) 51% <0%> (+12%)
...d/compute/v1/RegionInstanceGroupManagerClient.java 54.5% <0%> (-6.79%) 51% <0%> (+12%)
.../com/google/cloud/compute/v1/TargetPoolClient.java 57.07% <0%> (-6.73%) 47% <0%> (+11%)
... and 538 more

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 7a9f3fe...4a04862. Read the comment docs.

@meltsufin
Copy link
Copy Markdown
Member Author

meltsufin commented Apr 3, 2019

@sduskis Done.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Apr 4, 2019
@sduskis sduskis requested review from chingor13 and removed request for a team April 5, 2019 19:47
@dzou
Copy link
Copy Markdown
Contributor

dzou commented Apr 26, 2019

@chingor13 Hey, any updates regarding your thoughts on this PR?

chingor13
chingor13 approved these changes Apr 30, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

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

Let's take this for now and think about how we can potentially limit time spent flushing.

@chingor13 chingor13 merged commit aee1e2e into googleapis:master May 1, 2019
@meltsufin meltsufin mentioned this pull request May 1, 2019
Closed
@ajaaym ajaaym mentioned this pull request May 3, 2019
Closed
meltsufin added a commit that referenced this pull request Dec 22, 2025
@meltsufin @chingor13
Logging: call flush before close (#4784)
d740602 
* Logging: call flush before close

Also prevent more logs to be written after close.

Fixes #4783.

* code style fix
suztomo pushed a commit to suztomo/google-cloud-java that referenced this pull request Mar 11, 2026
@meltsufin @chingor13
Logging: call flush before close (googleapis#4784)
7231b00 
* Logging: call flush before close

Also prevent more logs to be written after close.

Fixes googleapis#4783.

* code style fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chingor13 chingor13 chingor13 approved these changes

Assignees

No one assigned

Labels

cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing. 🚨 This issue needs some love.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Logging pending writes not flushed on close causing hanging

6 participants

@meltsufin @sduskis @dzou @chingor13 @googlebot @yoshi-automation