aloyszhang commented on a change in pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#discussion_r550010559
##########
File path:
pulsar-broker/src/main/java/org/apache/pulsar/broker/intercept/ManagedLedgerInterceptorImpl.java
##########
@@ -83,9 +83,9 @@ public void onManagedLedgerPropertiesInitialize(Map<String,
String> propertiesMa
public void onManagedLedgerLastLedgerInitialize(String name, LedgerHandle
lh) {
try {
for (BrokerEntryMetadataInterceptor interceptor :
brokerEntryMetadataInterceptors) {
- if (interceptor instanceof AppendIndexMetadataInterceptor) {
+ if (interceptor instanceof AppendIndexMetadataInterceptor &&
lh.getLastAddConfirmed() >= 0) {
Review comment:
Agree with you that instanceof is not a good practice if we have a
better choice.
There are two class `AppendIndexMetadataInterceptor` and
`AppendBrokerTimestampMetadataInterceptor` implement
`BrokerEntryMetadataInterceptor`.
If using polymorphism, for `AppendBrokerTimestampMetadataInterceptor`, its
`recoveryIndexGenerator` method do nothing like
```java
public void recoveryIndexGenerator(long index) {
// do nothing.
}
```
And the new `onManagedLedgerLastLedgerInitialize` would be like
```java
@Override
public void onManagedLedgerLastLedgerInitialize(String name,
LedgerHandle lh) {
try {
for (BrokerEntryMetadataInterceptor interceptor :
brokerEntryMetadataInterceptors) {
if (lh.getLastAddConfirmed() >= 0) {
LedgerEntries ledgerEntries =
lh.read(lh.getLastAddConfirmed(),
lh.getLastAddConfirmed());
for (LedgerEntry entry : ledgerEntries) {
PulsarApi.BrokerEntryMetadata brokerEntryMetadata =
Commands.parseBrokerEntryMetadataIfExist(entry.getEntryBuffer());
if (brokerEntryMetadata != null &&
brokerEntryMetadata.hasIndex()) {
interceptor.recoveryIndexGenerator(brokerEntryMetadata.getIndex());
}
}
}
}
} catch (org.apache.bookkeeper.client.api.BKException |
InterruptedException e) {
log.error("[{}] Read last entry error.", name, e);
}
}
```
So using polymorphism, even if interceptor is a
`AppendBrokerTimestampMetadataInterceptor`, we still need to read the last
entry from BookKeeper ande deserialize the `BrokerEntryMetadata` from the
entry.
https://github.com/aloyszhang/pulsar/blob/recovery-index/pulsar-broker/src/main/java/org/apache/pulsar/broker/intercept/ManagedLedgerInterceptorImpl.java#L87~L92
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
