Make `jvm_memory_direct_bytes_used` metrics compatible with jdk8. by horizonzy · Pull Request #3677 · apache/bookkeeper

horizonzy · 2022-11-30T04:28:52Z

Descriptions of the changes in this PR:
In #3252, it only uses poolMxBeanOp to get direct memory usage. It only works in jdk11.
But if the user still uses the old version, netty still uses NoCleanerConstructor ByteBuffer to allocate direct memory, the poolMxBeanOp didn't cover it.
We should check the netty PlatformDependent.useDirectBufferNoCleaner(), then choose the corresponding way to get direct memory usage.

hezhangjian

Should we add another test case or just modify it?

horizonzy · 2022-11-30T06:20:36Z

Should we add another test case or just modify it?

Already change the test case to cover it.

horizonzy · 2022-11-30T06:21:09Z

rerun failure checks

hangc0276 · 2022-11-30T06:29:35Z

    private static final Optional<BufferPoolMXBean> poolMxBeanOp;

    static {
+        AtomicLong tmpDirectMemoryUsage = null;


Do we need to get direct memory usage here according to PlatformDependent.useDirectBufferNoCleaner() here instead of getting both values?

horizonzy · 2022-11-30T07:58:07Z

rerun failure checks

horizonzy · 2022-11-30T08:32:40Z

rerun failure checks

horizonzy · 2022-12-01T06:12:30Z

rerun failure checks

hangc0276 · 2022-12-05T02:14:53Z

+            List<BufferPoolMXBean> platformMXBeans = ManagementFactory.getPlatformMXBeans(BufferPoolMXBean.class);
+            poolMxBeanOp = platformMXBeans.stream()
+                    .filter(bufferPoolMXBean -> bufferPoolMXBean.getName().equals("direct")).findAny();
+            getDirectMemoryUsage = () -> poolMxBeanOp.get().getMemoryUsed();


We need to check whether poolMxBeanOp is present before get

hangc0276 · 2022-12-05T02:16:03Z

+                log.warn("Failed to access netty DIRECT_MEMORY_COUNTER field {}", t.getMessage());
+            }
+            directMemoryUsage = tmpDirectMemoryUsage;
+            getDirectMemoryUsage = () -> directMemoryUsage.get();


We need to check whether the directMemoryUsage is null before get

hangc0276 · 2022-12-05T02:16:24Z

+     */
+    private static AtomicLong directMemoryUsage;
+    private static Optional<BufferPoolMXBean> poolMxBeanOp;
+    private static Supplier<Long> getDirectMemoryUsage;


Can it be final?

hangc0276 · 2022-12-05T02:18:53Z

ping @eolivelli @dlg99 @zymap @shoothzj, please help take a look, thanks.

# Conflicts: # stats/bookkeeper-stats-providers/prometheus-metrics-provider/src/main/java/org/apache/bookkeeper/stats/prometheus/PrometheusMetricsProvider.java

horizonzy · 2022-12-05T06:19:27Z

rerun failure checks

zymap · 2022-12-07T02:34:32Z

+            poolMxBeanOp = Optional.empty();
+            AtomicLong tmpDirectMemoryUsage = null;
+            try {
+                Field field = PlatformDependent.class.getDeclaredField("DIRECT_MEMORY_COUNTER");


Looks like they have the usedDirectMemory() method to access that field. Why don't we use that?

eolivelli

LGTM

) (cherry picked from commit 5a38080)

Descriptions of the changes in this PR: ### Motivation Fix cherry-pick #3677 issue

…ache#3677) (cherry picked from commit 5a38080) (cherry picked from commit f73c7a1)

Descriptions of the changes in this PR: ### Motivation Fix cherry-pick apache#3677 issue (cherry picked from commit b454014)

…ache#3677)

) (cherry picked from commit 5a38080)

…ache#3677)

horizonzy added 3 commits November 30, 2022 12:12

Fix jvm memory metrics didn't work at jdk8.

5ad6e8e

Fix jvm memory metrics didn't work at jdk8.

333a177

fix checkstyle.

0fca759

hezhangjian reviewed Nov 30, 2022

View reviewed changes

hangc0276 reviewed Nov 30, 2022

View reviewed changes

horizonzy added 2 commits November 30, 2022 15:04

Address the comment.

8831d0e

fix checkstyle.

b7a778e

hangc0276 requested review from dlg99, eolivelli and zymap November 30, 2022 14:43

hangc0276 assigned horizonzy Nov 30, 2022

hangc0276 requested a review from hezhangjian November 30, 2022 14:43

hangc0276 added release/4.14.6 release/4.15.4 labels Nov 30, 2022

hangc0276 added this to the 4.16.0 milestone Nov 30, 2022

hangc0276 requested changes Dec 5, 2022

View reviewed changes

horizonzy added 2 commits December 5, 2022 11:15

Address the comment.

c030151

Merge branch 'master' into fix-3252

3b21cb9

# Conflicts: # stats/bookkeeper-stats-providers/prometheus-metrics-provider/src/main/java/org/apache/bookkeeper/stats/prometheus/PrometheusMetricsProvider.java

hezhangjian approved these changes Dec 5, 2022

View reviewed changes

hangc0276 approved these changes Dec 5, 2022

View reviewed changes

zymap reviewed Dec 7, 2022

View reviewed changes

eolivelli approved these changes Dec 7, 2022

View reviewed changes

eolivelli merged commit 5a38080 into apache:master Dec 7, 2022

hangc0276 pushed a commit that referenced this pull request Dec 8, 2022

Make jvm_memory_direct_bytes_used metrics compatible with jdk8. (#3677

f73c7a1

) (cherry picked from commit 5a38080)

hangc0276 added the cherry-picked/branch-4.14 label Dec 8, 2022

hangc0276 added a commit to hangc0276/bookkeeper that referenced this pull request Dec 8, 2022

fix cherry-pick apache#3677 issue

1be1d8b

hangc0276 mentioned this pull request Dec 8, 2022

[Branch-4.14] Fix cherry-pick #3677 issue #3699

Merged

hangc0276 added a commit that referenced this pull request Dec 9, 2022

fix cherry-pick #3677 issue (#3699)

b454014

Descriptions of the changes in this PR: ### Motivation Fix cherry-pick #3677 issue

nicoloboschi pushed a commit to datastax/bookkeeper that referenced this pull request Jan 11, 2023

Make jvm_memory_direct_bytes_used metrics compatible with jdk8. (ap…

66afe19

…ache#3677) (cherry picked from commit 5a38080) (cherry picked from commit f73c7a1)

nicoloboschi pushed a commit to datastax/bookkeeper that referenced this pull request Jan 11, 2023

fix cherry-pick apache#3677 issue (apache#3699)

a158b52

Descriptions of the changes in this PR: ### Motivation Fix cherry-pick apache#3677 issue (cherry picked from commit b454014)

yaalsn pushed a commit to yaalsn/bookkeeper that referenced this pull request Jan 30, 2023

Make jvm_memory_direct_bytes_used metrics compatible with jdk8. (ap…

27f82cd

…ache#3677)

zymap pushed a commit that referenced this pull request Feb 16, 2023

Make jvm_memory_direct_bytes_used metrics compatible with jdk8. (#3677

5df5d56

) (cherry picked from commit 5a38080)

zymap added the cherry-picked/branch-4.15 label Feb 17, 2023

Ghatage pushed a commit to sijie/bookkeeper that referenced this pull request Jul 12, 2024

Make jvm_memory_direct_bytes_used metrics compatible with jdk8. (ap…

0dc87d1

…ache#3677)

Uh oh!

Conversation

horizonzy commented Nov 30, 2022

Uh oh!

hezhangjian left a comment

Choose a reason for hiding this comment

Uh oh!

horizonzy commented Nov 30, 2022

Uh oh!

horizonzy commented Nov 30, 2022

Uh oh!

hangc0276 Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

horizonzy Nov 30, 2022

Choose a reason for hiding this comment

Uh oh!

horizonzy commented Nov 30, 2022

Uh oh!

horizonzy commented Nov 30, 2022

Uh oh!

horizonzy commented Dec 1, 2022

Uh oh!

hangc0276 Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

hangc0276 Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

hangc0276 Dec 5, 2022

Choose a reason for hiding this comment

Uh oh!

hangc0276 commented Dec 5, 2022

Uh oh!

horizonzy commented Dec 5, 2022

Uh oh!

zymap Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants