You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "ethqunzhong (via GitHub)" <gi...@apache.org> on 2023/05/22 03:33:24 UTC
[GitHub] [pulsar] ethqunzhong opened a new pull request, #20366: [fix][offload] fix offload metrics error
ethqunzhong opened a new pull request, #20366:
URL: https://github.com/apache/pulsar/pull/20366
Related issue #20355
### Motivation
The current implementation has the following issues:
1. When constructing offload metrics, using the managedLeger name as topicName results in an incorrect format, causing abnormal display of metrics on Grafana.
2. When calculating offload bytes for Filesystem offload, it incorrectly calculates the actual write value, resulting in much larger metrics than the actual value.
### Modifications
1. Add the `fromPersistenceNamingEncoding` method to `TopicName` and use mlname to obtain the topic name.
2. Replace the part of using mlname in the logic of topic label before offload metrics with using topicname.
3. Modify the logic of `recordOffloadBytes` in filesystem offloader, change `entry.getLength()` to `entry.getEntryBytes().length`.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
This change is already covered by existing tests.
### Does this pull request potentially affect one of the following parts:
*If the box was checked, please highlight the changes*
- [x] 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` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
### Matching PR in forked repository
PR in forked repository: https://github.com/ethqunzhong/pulsar/pull/6
--
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] ethqunzhong commented on pull request #20366: [fix][offload] fix offload metrics error
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#issuecomment-1557013336
/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] lhotari commented on a diff in pull request #20366: [fix][offload] fix offload metrics error
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#discussion_r1209693521
##########
tiered-storage/file-system/src/main/java/org/apache/bookkeeper/mledger/offload/filesystem/impl/FileSystemManagedLedgerOffloader.java:
##########
@@ -306,22 +308,25 @@ public static FileSystemWriter create(LedgerEntries ledgerEntriesOnce,
@Override
public void run() {
String managedLedgerName = ledgerReader.extraMetadata.get(MANAGED_LEDGER_NAME);
+ String topicName = TopicName.fromPersistenceNamingEncoding(managedLedgerName);
if (ledgerReader.fileSystemWriteException == null) {
Iterator<LedgerEntry> iterator = ledgerEntriesOnce.iterator();
while (iterator.hasNext()) {
LedgerEntry entry = iterator.next();
long entryId = entry.getEntryId();
key.set(entryId);
+ int currentEntrySize;
try {
- value.set(entry.getEntryBytes(), 0, entry.getEntryBytes().length);
+ currentEntrySize = entry.getEntryBytes().length;
Review Comment:
store `entry.getEntryBytes()` to a local variable so that it doesn't get called twice.
--
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] poorbarcode commented on pull request #20366: [fix][offload] fix offload metrics error
Posted by "poorbarcode (via GitHub)" <gi...@apache.org>.
poorbarcode commented on PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#issuecomment-1568097065
@hangc0276 Could you take a look?
--
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] tjiuming commented on pull request #20366: [fix][offload] fix offload metrics error
Posted by "tjiuming (via GitHub)" <gi...@apache.org>.
tjiuming commented on PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#issuecomment-1614101984
good catch!
--
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] ethqunzhong commented on pull request #20366: [fix][offload] fix offload metrics error
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#issuecomment-1569508890
/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] ethqunzhong commented on pull request #20366: [fix][offload] fix offload metrics error
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#issuecomment-1562205521
@tjiuming PTAL
--
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 #20366: [fix][offload] fix offload metrics error
Posted by "hangc0276 (via GitHub)" <gi...@apache.org>.
hangc0276 commented on code in PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#discussion_r1210085410
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -339,6 +339,42 @@ public String getPersistenceNamingEncoding() {
}
}
+ /**
+ * get topic full name from managedLedgerName.
+ *
+ * @return the topic full name, format -> domain://tenant/namespace/topic
+ */
+ public static String fromPersistenceNamingEncoding(String mlName) {
Review Comment:
Please add a unit test to cover this method.
--
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] ethqunzhong commented on pull request #20366: [fix][offload] fix offload metrics error
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#issuecomment-1559124623
@eolivelli @dlg99 all test passed. PTAL
--
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] ethqunzhong commented on a diff in pull request #20366: [fix][offload] fix offload metrics error
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on code in PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#discussion_r1209711540
##########
tiered-storage/file-system/src/main/java/org/apache/bookkeeper/mledger/offload/filesystem/impl/FileSystemManagedLedgerOffloader.java:
##########
@@ -306,22 +308,25 @@ public static FileSystemWriter create(LedgerEntries ledgerEntriesOnce,
@Override
public void run() {
String managedLedgerName = ledgerReader.extraMetadata.get(MANAGED_LEDGER_NAME);
+ String topicName = TopicName.fromPersistenceNamingEncoding(managedLedgerName);
if (ledgerReader.fileSystemWriteException == null) {
Iterator<LedgerEntry> iterator = ledgerEntriesOnce.iterator();
while (iterator.hasNext()) {
LedgerEntry entry = iterator.next();
long entryId = entry.getEntryId();
key.set(entryId);
+ int currentEntrySize;
try {
- value.set(entry.getEntryBytes(), 0, entry.getEntryBytes().length);
+ currentEntrySize = entry.getEntryBytes().length;
Review Comment:
@lhotari Thanks for your reminder. updated.
--
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] ethqunzhong commented on a diff in pull request #20366: [fix][offload] fix offload metrics error
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on code in PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#discussion_r1209236179
##########
tiered-storage/file-system/src/main/java/org/apache/bookkeeper/mledger/offload/filesystem/impl/FileSystemManagedLedgerOffloader.java:
##########
@@ -317,11 +320,11 @@ public void run() {
dataWriter.append(key, value);
} catch (IOException e) {
ledgerReader.fileSystemWriteException = e;
- ledgerReader.offloaderStats.recordWriteToStorageError(managedLedgerName);
+ ledgerReader.offloaderStats.recordWriteToStorageError(topicName);
break;
}
haveOffloadEntryNumber.incrementAndGet();
- ledgerReader.offloaderStats.recordOffloadBytes(managedLedgerName, entry.getLength());
+ ledgerReader.offloaderStats.recordOffloadBytes(topicName, entry.getEntryBytes().length);
Review Comment:
<img width="971" alt="image" src="https://github.com/apache/pulsar/assets/16517186/f0b91ead-dac5-450a-9f31-a86b0f54b7da">
From the perspective of entry implementation, using entry.getLength() to record metrics will result in duplicated calculations of data.
```
while (iterator.hasNext()) {
LedgerEntry entry = iterator.next();
...
ledgerReader.offloaderStats.recordOffloadBytes(topicName, entry.getLength());
}
```
This will cause the metrics to be much larger than the actual values.
To reduce expensive calls, I made the following adjustments. Please help me review them again.
```
while (iterator.hasNext()) {
int currentEntrySize;
try {
currentEntrySize = entry.getEntryBytes().length;
...
} catch (IOException e) {
...
break;
}
haveOffloadEntryNumber.incrementAndGet();
ledgerReader.offloaderStats.recordOffloadBytes(topicName, currentEntrySize);
}
```
--
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] codelipenghui merged pull request #20366: [fix][offload] fix offload metrics error
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui merged PR #20366:
URL: https://github.com/apache/pulsar/pull/20366
--
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] codecov-commenter commented on pull request #20366: [fix][offload] fix offload metrics error
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#issuecomment-1557023329
## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#20366](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (140147c) into [master](https://app.codecov.io/gh/apache/pulsar/commit/426ad3e53ba621c442bce89eb208add8f9d1563e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (426ad3e) will **decrease** coverage by `0.07%`.
> The diff coverage is `63.26%`.
[](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #20366 +/- ##
============================================
- Coverage 72.93% 72.86% -0.07%
+ Complexity 31973 31869 -104
============================================
Files 1868 1864 -4
Lines 138663 138413 -250
Branches 15248 15186 -62
============================================
- Hits 101133 100859 -274
- Misses 29484 29541 +57
+ Partials 8046 8013 -33
```
| Flag | Coverage Δ | |
|---|---|---|
| inttests | `24.23% <0.00%> (+0.11%)` | :arrow_up: |
| systests | `24.92% <38.88%> (+0.05%)` | :arrow_up: |
| unittests | `72.16% <63.26%> (-0.07%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ache/pulsar/broker/loadbalance/LinuxInfoUtils.java](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9MaW51eEluZm9VdGlscy5qYXZh) | `43.65% <ø> (ø)` | |
| [...ker/loadbalance/impl/LinuxBrokerHostUsageImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9pbXBsL0xpbnV4QnJva2VySG9zdFVzYWdlSW1wbC5qYXZh) | `84.37% <ø> (ø)` | |
| [...d/jcloud/impl/BlobStoreManagedLedgerOffloader.java](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dGllcmVkLXN0b3JhZ2UvamNsb3VkL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9ib29ra2VlcGVyL21sZWRnZXIvb2ZmbG9hZC9qY2xvdWQvaW1wbC9CbG9iU3RvcmVNYW5hZ2VkTGVkZ2VyT2ZmbG9hZGVyLmphdmE=) | `75.06% <33.33%> (-0.32%)` | :arrow_down: |
| [...ava/org/apache/pulsar/common/naming/TopicName.java](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWNvbW1vbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9uYW1pbmcvVG9waWNOYW1lLmphdmE=) | `88.97% <44.44%> (-7.36%)` | :arrow_down: |
| [...esystem/impl/FileSystemManagedLedgerOffloader.java](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dGllcmVkLXN0b3JhZ2UvZmlsZS1zeXN0ZW0vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9vZmZsb2FkL2ZpbGVzeXN0ZW0vaW1wbC9GaWxlU3lzdGVtTWFuYWdlZExlZGdlck9mZmxvYWRlci5qYXZh) | `67.70% <66.66%> (+0.51%)` | :arrow_up: |
| [...filesystem/impl/FileStoreBackedReadHandleImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dGllcmVkLXN0b3JhZ2UvZmlsZS1zeXN0ZW0vc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Jvb2trZWVwZXIvbWxlZGdlci9vZmZsb2FkL2ZpbGVzeXN0ZW0vaW1wbC9GaWxlU3RvcmVCYWNrZWRSZWFkSGFuZGxlSW1wbC5qYXZh) | `56.47% <80.00%> (+0.51%)` | :arrow_up: |
| [...ad/jcloud/impl/BlobStoreBackedInputStreamImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dGllcmVkLXN0b3JhZ2UvamNsb3VkL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9ib29ra2VlcGVyL21sZWRnZXIvb2ZmbG9hZC9qY2xvdWQvaW1wbC9CbG9iU3RvcmVCYWNrZWRJbnB1dFN0cmVhbUltcGwuamF2YQ==) | `97.40% <100.00%> (+0.03%)` | :arrow_up: |
| [...oad/jcloud/impl/BlobStoreBackedReadHandleImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dGllcmVkLXN0b3JhZ2UvamNsb3VkL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9ib29ra2VlcGVyL21sZWRnZXIvb2ZmbG9hZC9qY2xvdWQvaW1wbC9CbG9iU3RvcmVCYWNrZWRSZWFkSGFuZGxlSW1wbC5qYXZh) | `80.14% <100.00%> (+0.14%)` | :arrow_up: |
| [...d/jcloud/impl/BlobStoreBackedReadHandleImplV2.java](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dGllcmVkLXN0b3JhZ2UvamNsb3VkL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9ib29ra2VlcGVyL21sZWRnZXIvb2ZmbG9hZC9qY2xvdWQvaW1wbC9CbG9iU3RvcmVCYWNrZWRSZWFkSGFuZGxlSW1wbFYyLmphdmE=) | `61.14% <100.00%> (+0.24%)` | :arrow_up: |
| [.../jcloud/impl/BlockAwareSegmentInputStreamImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/20366?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-dGllcmVkLXN0b3JhZ2UvamNsb3VkL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9ib29ra2VlcGVyL21sZWRnZXIvb2ZmbG9hZC9qY2xvdWQvaW1wbC9CbG9ja0F3YXJlU2VnbWVudElucHV0U3RyZWFtSW1wbC5qYXZh) | `90.00% <100.00%> (+0.06%)` | :arrow_up: |
... and [95 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/20366/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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] lhotari commented on a diff in pull request #20366: [fix][offload] fix offload metrics error
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#discussion_r1209693521
##########
tiered-storage/file-system/src/main/java/org/apache/bookkeeper/mledger/offload/filesystem/impl/FileSystemManagedLedgerOffloader.java:
##########
@@ -306,22 +308,25 @@ public static FileSystemWriter create(LedgerEntries ledgerEntriesOnce,
@Override
public void run() {
String managedLedgerName = ledgerReader.extraMetadata.get(MANAGED_LEDGER_NAME);
+ String topicName = TopicName.fromPersistenceNamingEncoding(managedLedgerName);
if (ledgerReader.fileSystemWriteException == null) {
Iterator<LedgerEntry> iterator = ledgerEntriesOnce.iterator();
while (iterator.hasNext()) {
LedgerEntry entry = iterator.next();
long entryId = entry.getEntryId();
key.set(entryId);
+ int currentEntrySize;
try {
- value.set(entry.getEntryBytes(), 0, entry.getEntryBytes().length);
+ currentEntrySize = entry.getEntryBytes().length;
Review Comment:
yes, having a local variable `currentEntrySize` is a useful solution here. Please also
store `entry.getEntryBytes()` to a local variable so that it doesn't get called twice.
--
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] ethqunzhong commented on a diff in pull request #20366: [fix][offload] fix offload metrics error
Posted by "ethqunzhong (via GitHub)" <gi...@apache.org>.
ethqunzhong commented on code in PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#discussion_r1210287895
##########
pulsar-common/src/main/java/org/apache/pulsar/common/naming/TopicName.java:
##########
@@ -339,6 +339,42 @@ public String getPersistenceNamingEncoding() {
}
}
+ /**
+ * get topic full name from managedLedgerName.
+ *
+ * @return the topic full name, format -> domain://tenant/namespace/topic
+ */
+ public static String fromPersistenceNamingEncoding(String mlName) {
Review Comment:
Unit testing has been added. PTAL @hangc0276
--
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] lhotari commented on a diff in pull request #20366: [fix][offload] fix offload metrics error
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #20366:
URL: https://github.com/apache/pulsar/pull/20366#discussion_r1209036406
##########
tiered-storage/file-system/src/main/java/org/apache/bookkeeper/mledger/offload/filesystem/impl/FileSystemManagedLedgerOffloader.java:
##########
@@ -317,11 +320,11 @@ public void run() {
dataWriter.append(key, value);
} catch (IOException e) {
ledgerReader.fileSystemWriteException = e;
- ledgerReader.offloaderStats.recordWriteToStorageError(managedLedgerName);
+ ledgerReader.offloaderStats.recordWriteToStorageError(topicName);
break;
}
haveOffloadEntryNumber.incrementAndGet();
- ledgerReader.offloaderStats.recordOffloadBytes(managedLedgerName, entry.getLength());
+ ledgerReader.offloaderStats.recordOffloadBytes(topicName, entry.getEntryBytes().length);
Review Comment:
Why wasn't the call `entry.getLength()` sufficient?
`entry.getEntryBytes().length` is an expensive call. `entry.getEntryBuffer().readableBytes()` would be an option if `entry.getLength()` doesn't produce the correct value.
--
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