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

[fix] StatsOutputStream: add string write function (#308) #23227

Merged
merged 1 commit into from
Aug 26, 2024

Conversation

dlg99
Copy link
Contributor

@dlg99 dlg99 commented Aug 26, 2024

Motivation

On a specific environment /metrics end point timed out consistently, logs contained the exception below.
Applied fix has resolved the issue.

The fix does not make much sense as String implements CharSequence, so I suspect we hit some weird issue with JIT or jvm.
The JDK is Timurin build 17.0.12+7, I could not repro this locally but it consistently happened in the env.

2024-08-23T20:40:42,618+0000 [pulsar-stats-updater-OrderedScheduler-0-0] WARN  org.apache.bookkeeper.common.util.SingleThreadSafeScheduledExecutorService - Unexpected throwable from task class com.google.common.util.concurrent.MoreExecutors$ScheduledListeningDecorator$NeverSuccessfulListenableFutureTask: 'org.apache.pulsar.common.util.SimpleTextOutputStream org.apache.pulsar.common.util.SimpleTextOutputStream.write(java.lang.String)'
java.lang.NoSuchMethodError: 'org.apache.pulsar.common.util.SimpleTextOutputStream org.apache.pulsar.common.util.SimpleTextOutputStream.write(java.lang.String)'
        at org.apache.pulsar.utils.StatsOutputStream.startObject(StatsOutputStream.java:41) ~[com.datastax.oss-pulsar-broker-3.1.3.1.jar:3.1.3.1]
        at org.apache.pulsar.broker.service.PulsarStats.lambda$updateStats$4(PulsarStats.java:126) ~[com.datastax.oss-pulsar-broker-3.1.3.1.jar:3.1.3.1]
        at org.apache.pulsar.common.util.collections.ConcurrentOpenHashMap$Section.forEach(ConcurrentOpenHashMap.java:563) ~[com.datastax.oss-pulsar-common-3.1.3.1.jar:3.1.3.1]
        at org.apache.pulsar.common.util.collections.ConcurrentOpenHashMap.forEach(ConcurrentOpenHashMap.java:277) ~[com.datastax.oss-pulsar-common-3.1.3.1.jar:3.1.3.1]
        at org.apache.pulsar.broker.service.PulsarStats.updateStats(PulsarStats.java:120) ~[com.datastax.oss-pulsar-broker-3.1.3.1.jar:3.1.3.1]
        at org.apache.pulsar.broker.service.BrokerService.updateRates(BrokerService.java:2065) ~[com.datastax.oss-pulsar-broker-3.1.3.1.jar:3.1.3.1]
        at com.google.common.util.concurrent.MoreExecutors$ScheduledListeningDecorator$NeverSuccessfulListenableFutureTask.run(MoreExecutors.java:741) ~[com.google.guava-guava-32.1.1-jre.jar:?]
        at org.apache.bookkeeper.common.util.SingleThreadSafeScheduledExecutorService$SafeRunnable.run(SingleThreadSafeScheduledExecutorService.java:46) ~[org.apache.bookkeeper-bookkeeper-common-4.16.4.jar:4.16.4]
        at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) ~[?:?]
        at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305) ~[?:?]
        at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
        at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) ~[io.netty-netty-common-4.1.108.Final.jar:4.1.108.Final]
        at java.lang.Thread.run(Thread.java:840) ~[?:?]

Modifications

Added method that takes String parameter explicitly.
Such method existed up until the change #22494

Verifying this change

  • Make sure that the change passes the CI checks.

Verified in the env where the problem reproduces

Does this pull request potentially affect one of the following parts:

NO

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: datastax#308

This function was recently changed causing a failure in broker metrics.

(cherry picked from commit 751aea3)
@dlg99 dlg99 requested a review from lhotari August 26, 2024 18:03
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 26, 2024
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM. I guess that one possible explanation is plugin code that is compiled without the changes. Anyhow, makes sense to restore binary compatibility.
nit: The implementation could also delegate to the method that takes a CharSequence instead of duplicating the code.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.52%. Comparing base (bbc6224) to head (a6e1075).
Report is 545 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23227      +/-   ##
============================================
+ Coverage     73.57%   74.52%   +0.95%     
- Complexity    32624    34225    +1601     
============================================
  Files          1877     1922      +45     
  Lines        139502   144758    +5256     
  Branches      15299    15832     +533     
============================================
+ Hits         102638   107884    +5246     
+ Misses        28908    28608     -300     
- Partials       7956     8266     +310     
Flag Coverage Δ
inttests 27.66% <50.00%> (+3.08%) ⬆️
systests 24.67% <50.00%> (+0.34%) ⬆️
unittests 73.88% <100.00%> (+1.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...che/pulsar/common/util/SimpleTextOutputStream.java 88.40% <100.00%> (-1.25%) ⬇️

... and 535 files with indirect coverage changes

@lhotari lhotari merged commit cd3519a into apache:master Aug 26, 2024
53 of 54 checks passed
@dlg99 dlg99 deleted the str-write-metrics branch August 26, 2024 20:33
lhotari pushed a commit that referenced this pull request Aug 28, 2024
Co-authored-by: Paul Gier <paul.gier@datastax.com>
(cherry picked from commit cd3519a)
lhotari pushed a commit that referenced this pull request Aug 28, 2024
Co-authored-by: Paul Gier <paul.gier@datastax.com>
(cherry picked from commit cd3519a)
grssam pushed a commit to grssam/pulsar that referenced this pull request Sep 4, 2024
@lhotari lhotari added this to the 4.0.0 milestone Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants