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 2020/12/30 06:27:04 UTC
[GitHub] [pulsar] aloyszhang opened a new pull request #9091: fix BKIncorrectParameterException
aloyszhang opened a new pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091
Fixes #9090
### Motivation
This PR add pre-condition before read entry from BookKeeper to fix BKIncorrectParameterException
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-752685754
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
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 , 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
```java
LedgerEntries ledgerEntries =
lh.read(lh.getLastAddConfirmed(), lh.getLastAddConfirmed());
for (LedgerEntry entry : ledgerEntries) {
PulsarApi.BrokerEntryMetadata brokerEntryMetadata =
Commands.parseBrokerEntryMetadataIfExist(entry.getEntryBuffer());
if (brokerEntryMetadata != null && brokerEntryMetadata.hasIndex()) {
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
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 and the interceptor is a `AppendBrokerTimestampMetadataInterceptor` whose `recoveryIndexGenerator` method do nothing like
```java
public void recoveryIndexGenerator(long index) {
// do nothing.
}
```
, we still meed to deserialize the `BrokerEntryMetadata` from entry.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-752634717
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#discussion_r549954153
##########
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:
Using instanceof is a bad practice, not Object oriented.
It is not in the scope of this PR but it would be better to have a more OO approach, using polymorphism
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-752375838
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang removed a comment on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang removed a comment on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-752413402
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
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 , even if interceptor is a `AppendBrokerTimestampMetadataInterceptor`, we still need to read the last entry from BookKeeper and deserialize `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
```java
LedgerEntries ledgerEntries =
lh.read(lh.getLastAddConfirmed(), lh.getLastAddConfirmed());
for (LedgerEntry entry : ledgerEntries) {
PulsarApi.BrokerEntryMetadata brokerEntryMetadata =
Commands.parseBrokerEntryMetadataIfExist(entry.getEntryBuffer());
if (brokerEntryMetadata != null && brokerEntryMetadata.hasIndex()) {
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-752857270
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
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
```java
LedgerEntries ledgerEntries =
lh.read(lh.getLastAddConfirmed(), lh.getLastAddConfirmed());
for (LedgerEntry entry : ledgerEntries) {
PulsarApi.BrokerEntryMetadata brokerEntryMetadata =
Commands.parseBrokerEntryMetadataIfExist(entry.getEntryBuffer());
if (brokerEntryMetadata != null && brokerEntryMetadata.hasIndex()) {
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang removed a comment on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang removed a comment on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-752634717
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-753617299
@aloyszhang please rebase the master branch
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-752818070
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-753253373
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on a change in pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#discussion_r550016745
##########
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:
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie merged pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
sijie merged pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-753737701
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-752413402
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on a change in pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
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 , even if interceptor is a `AppendBrokerTimestampMetadataInterceptor`, we still need to read the last entry from BookKeeper and deserialize `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
```java
LedgerEntries ledgerEntries =
lh.read(lh.getLastAddConfirmed(), lh.getLastAddConfirmed());
for (LedgerEntry entry : ledgerEntries) {
PulsarApi.BrokerEntryMetadata brokerEntryMetadata =
Commands.parseBrokerEntryMetadataIfExist(entry.getEntryBuffer());
if (brokerEntryMetadata != null && brokerEntryMetadata.hasIndex()) {
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [pulsar] aloyszhang commented on pull request #9091: fix BKIncorrectParameterException
Posted by GitBox <gi...@apache.org>.
aloyszhang commented on pull request #9091:
URL: https://github.com/apache/pulsar/pull/9091#issuecomment-752794193
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org