You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/07/05 13:46:20 UTC
[GitHub] [pulsar] eolivelli opened a new pull request, #16405: Offloaders: fix metrics
eolivelli opened a new pull request, #16405:
URL: https://github.com/apache/pulsar/pull/16405
### Motivation
There are a few problems with Offloader metrics:
- the scheduler does not run
- JClouds read latency is not calculated well, because the whole read time must be taken after streaming the whole HTTP payload
- we miss a raw counter of the amount of data read from Tiered Storage
### Modifications
- pass the Scheduler for periodic calculations
- add raw brk_ledgeroffloader_read_bytes counter
- fix read latency from JClouds calculation
### Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #16405:
URL: https://github.com/apache/pulsar/pull/16405#issuecomment-1183228948
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] horizonzy commented on a diff in pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
horizonzy commented on code in PR #16405:
URL: https://github.com/apache/pulsar/pull/16405#discussion_r918651003
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java:
##########
@@ -734,8 +734,17 @@ public void start() throws PulsarServerException {
schemaRegistryService = SchemaRegistryService.create(
schemaStorage, config.getSchemaRegistryCompatibilityCheckers(), this.executor);
- this.defaultOffloader = createManagedLedgerOffloader(
- OffloadPoliciesImpl.create(this.getConfiguration().getProperties()));
+ OffloadPoliciesImpl defaultOffloadPolicies =
+ OffloadPoliciesImpl.create(this.getConfiguration().getProperties());
+ this.defaultOffloader = createManagedLedgerOffloader(defaultOffloadPolicies);
Review Comment:
We should make
```
this.defaultOffloader = createManagedLedgerOffloader(defaultOffloadPolicies);
```
after
```
offloaderStats = LedgerOffloaderStats.create(config.isExposeManagedLedgerMetricsInPrometheus(),
exposeTopicMetrics, offloaderScheduler, interval);
```
In `createManagedLedgerOffloader`, it will use `offloaderStats`. We shuold make this method after `offloaderStats` override.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on a diff in pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on code in PR #16405:
URL: https://github.com/apache/pulsar/pull/16405#discussion_r914714869
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/LedgerOffloaderStatsImpl.java:
##########
@@ -86,15 +88,27 @@ private LedgerOffloaderStatsImpl(boolean exposeTopicLevelMetrics,
.labelNames(labels).create().register();
this.readOffloadRate = Gauge.build("brk_ledgeroffloader_read_offload_rate", "-")
.labelNames(labels).create().register();
+ this.readOffloadBytes = Counter.build("brk_ledgeroffloader_read_bytes", "-")
+ .labelNames(labels).create().register();
this.writeStorageError = Counter.build("brk_ledgeroffloader_write_storage_error", "-")
.labelNames(labels).create().register();
this.readOffloadIndexLatency = Summary.build("brk_ledgeroffloader_read_offload_index_latency", "-")
- .labelNames(labels).create().register();
+ .labelNames(labels).quantile(0.50, 0.01)
+ .quantile(0.95, 0.01)
+ .quantile(0.99, 0.01)
Review Comment:
Do we need add `quantile(1.0, 0.01)`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #16405:
URL: https://github.com/apache/pulsar/pull/16405#issuecomment-1182957504
@codelipenghui @nicoloboschi @Technoboy-
the error in the test (broker group 1) is unrelated to this patch
> Error: Tests run: 43, Failures: 1, Errors: 0, Skipped: 9, Time elapsed: 51.938 s <<< FAILURE! - in org.apache.pulsar.broker.service.PersistentTopicTest
> Error: setup(org.apache.pulsar.broker.service.PersistentTopicTest) Time elapsed: 5.343 s <<< FAILURE!
> org.mockito.exceptions.base.MockitoException: Unable to create mock instance of type 'ServerCnx'
> at org.apache.pulsar.broker.BrokerTestUtil.spyWithClassAndConstructorArgs(BrokerTestUtil.java:43)
> at org.apache.pulsar.broker.service.PersistentTopicTest.setup(PersistentTopicTest.java:226)
> at jdk.internal.reflect.GeneratedMethodAccessor576.invoke(Unknown Source)
> at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
> at java.base/java.lang.reflect.Method.invoke(Method.java:568)
> at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
> at org.testng.internal.MethodInvocationHelper.invokeMethodConsideringTimeout(MethodInvocationHelper.java:61)
> at org.testng.internal.ConfigInvoker.invokeConfigurationMethod(ConfigInvoker.java:366)
> at org.testng.internal.ConfigInvoker.invokeConfigurations(ConfigInvoker.java:320)
> at org.testng.internal.TestInvoker.runConfigMethods(TestInvoker.java:701)
> at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:527)
> at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
> at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
> at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822)
> at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
> at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
> at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)
> at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
> at org.testng.TestRunner.privateRun(TestRunner.java:764)
> at org.testng.TestRunner.run(TestRunner.java:585)
> at org.testng.SuiteRunner.runTest(SuiteRunner.java:384)
> at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378)
> at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337)
> at org.testng.SuiteRunner.run(SuiteRunner.java:286)
> at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53)
> at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96)
> at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218)
> at org.testng.TestNG.runSuitesLocally(TestNG.java:1140)
> at org.testng.TestNG.runSuites(TestNG.java:1069)
> at org.testng.TestNG.run(TestNG.java:1037)
> at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:135)
> at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeSingleClass(TestNGDirectoryTestSuite.java:112)
> at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeLazy(TestNGDirectoryTestSuite.java:123)
> at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:90)
> at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:146)
> at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
> at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
> at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
> at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
> Caused by: org.mockito.creation.instance.InstantiationException:
> Unable to create instance of 'ServerCnx'.
> Please ensure the target class has a constructor that matches these argument types: [org.apache.pulsar.broker.PulsarService] and executes cleanly.
> ... 39 more
> Caused by: java.lang.reflect.InvocationTargetException
> at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:198)
> at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:161)
> at org.mockito.internal.util.reflection.ModuleMemberAccessor.newInstance(ModuleMemberAccessor.java:42)
> at org.mockito.internal.creation.instance.ConstructorInstantiator.invokeConstructor(ConstructorInstantiator.java:70)
> at org.mockito.internal.creation.instance.ConstructorInstantiator.withParams(ConstructorInstantiator.java:53)
> at org.mockito.internal.creation.instance.ConstructorInstantiator.newInstance(ConstructorInstantiator.java:39)
> at org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.doCreateMock(InlineDelegateByteBuddyMockMaker.java:360)
> at org.mockito.internal.creation.bytebuddy.InlineDelegateByteBuddyMockMaker.createMock(InlineDelegateByteBuddyMockMaker.java:330)
> at org.mockito.internal.creation.bytebuddy.InlineByteBuddyMockMaker.createMock(InlineByteBuddyMockMaker.java:58)
> at org.mockito.internal.util.MockUtil.createMock(MockUtil.java:53)
> at org.mockito.internal.MockitoCore.mock(MockitoCore.java:84)
> at org.mockito.Mockito.mock(Mockito.java:1[964](https://github.com/apache/pulsar/runs/7315483388?check_suite_focus=true#step:10:965))
> ... 39 more
> Caused by: java.lang.ClassCastException: class org.apache.pulsar.broker.service.BrokerService cannot be cast to class org.apache.pulsar.broker.resources.PulsarResources (org.apache.pulsar.broker.service.BrokerService and org.apache.pulsar.broker.resources.PulsarResources are in unnamed module of loader 'app')
> at org.apache.pulsar.broker.PulsarService.getPulsarResources(PulsarService.java:265)
> at org.apache.pulsar.broker.service.TopicListService.<init>(TopicListService.java:103)
> at org.apache.pulsar.broker.service.ServerCnx.<init>(ServerCnx.java:279)
> at org.apache.pulsar.broker.service.ServerCnx.<init>(ServerCnx.java:239)
> at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:732)
> at org.mockito.internal.util.reflection.InstrumentationMemberAccessor$Dispatcher$ByteBuddy$rxYoJ6WV.invokeWithArguments(Unknown Source)
> at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.lambda$newInstance$0(InstrumentationMemberAccessor.java:191)
> at org.mockito.internal.util.reflection.InstrumentationMemberAccessor.newInstance(InstrumentationMemberAccessor.java:188)
> ... 50 more
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16405:
URL: https://github.com/apache/pulsar/pull/16405#discussion_r915018080
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/LedgerOffloaderStatsImpl.java:
##########
@@ -86,15 +88,27 @@ private LedgerOffloaderStatsImpl(boolean exposeTopicLevelMetrics,
.labelNames(labels).create().register();
this.readOffloadRate = Gauge.build("brk_ledgeroffloader_read_offload_rate", "-")
.labelNames(labels).create().register();
+ this.readOffloadBytes = Counter.build("brk_ledgeroffloader_read_bytes", "-")
+ .labelNames(labels).create().register();
this.writeStorageError = Counter.build("brk_ledgeroffloader_write_storage_error", "-")
.labelNames(labels).create().register();
this.readOffloadIndexLatency = Summary.build("brk_ledgeroffloader_read_offload_index_latency", "-")
- .labelNames(labels).create().register();
+ .labelNames(labels).quantile(0.50, 0.01)
+ .quantile(0.95, 0.01)
+ .quantile(0.99, 0.01)
Review Comment:
added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli merged pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
eolivelli merged PR #16405:
URL: https://github.com/apache/pulsar/pull/16405
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16405:
URL: https://github.com/apache/pulsar/pull/16405#discussion_r915013499
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/LedgerOffloaderStatsImpl.java:
##########
@@ -86,15 +88,27 @@ private LedgerOffloaderStatsImpl(boolean exposeTopicLevelMetrics,
.labelNames(labels).create().register();
this.readOffloadRate = Gauge.build("brk_ledgeroffloader_read_offload_rate", "-")
.labelNames(labels).create().register();
+ this.readOffloadBytes = Counter.build("brk_ledgeroffloader_read_bytes", "-")
+ .labelNames(labels).create().register();
this.writeStorageError = Counter.build("brk_ledgeroffloader_write_storage_error", "-")
.labelNames(labels).create().register();
this.readOffloadIndexLatency = Summary.build("brk_ledgeroffloader_read_offload_index_latency", "-")
- .labelNames(labels).create().register();
+ .labelNames(labels).quantile(0.50, 0.01)
+ .quantile(0.95, 0.01)
+ .quantile(0.99, 0.01)
Review Comment:
added
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16405:
URL: https://github.com/apache/pulsar/pull/16405#discussion_r917852176
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/LedgerOffloaderStatsImpl.java:
##########
@@ -31,12 +31,13 @@
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.LongAdder;
import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
import org.apache.bookkeeper.mledger.LedgerOffloaderStats;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.tuple.ImmutablePair;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.pulsar.common.naming.TopicName;
-
+@Slf4j
Review Comment:
good catch. removed
@zymap
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on a diff in pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
zymap commented on code in PR #16405:
URL: https://github.com/apache/pulsar/pull/16405#discussion_r917557906
##########
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/LedgerOffloaderStatsImpl.java:
##########
@@ -31,12 +31,13 @@
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.LongAdder;
import java.util.stream.Collectors;
+import lombok.extern.slf4j.Slf4j;
import org.apache.bookkeeper.mledger.LedgerOffloaderStats;
import org.apache.commons.lang3.StringUtils;
import org.apache.commons.lang3.tuple.ImmutablePair;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.pulsar.common.naming.TopicName;
-
+@Slf4j
Review Comment:
Unused import?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on PR #16405:
URL: https://github.com/apache/pulsar/pull/16405#issuecomment-1179984292
ping @tjiuming @zymap @horizonzy, Please help take a look at this pr, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli closed pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #16405: Offloaders: fix metrics
URL: https://github.com/apache/pulsar/pull/16405
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli closed pull request #16405: Offloaders: fix metrics
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #16405: Offloaders: fix metrics
URL: https://github.com/apache/pulsar/pull/16405
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org