-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix BKIncorrectParameterException #9091
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
public void recoveryIndexGenerator(long index) {
// do nothing.
}
And the new onManagedLedgerLastLedgerInitialize
would be like
@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
LedgerEntries ledgerEntries =
lh.read(lh.getLastAddConfirmed(), lh.getLastAddConfirmed());
for (LedgerEntry entry : ledgerEntries) {
PulsarApi.BrokerEntryMetadata brokerEntryMetadata =
Commands.parseBrokerEntryMetadataIfExist(entry.getEntryBuffer());
if (brokerEntryMetadata != null && brokerEntryMetadata.hasIndex()) {
/pulsarbot run-failure-checks |
/pulsarbot run-failure-checks |
3 similar comments
/pulsarbot run-failure-checks |
/pulsarbot run-failure-checks |
/pulsarbot run-failure-checks |
@aloyszhang please rebase the master branch |
80bec8f
to
3d3ab5b
Compare
/pulsarbot run-failure-checks |
Fixes #9090
Motivation
This PR add pre-condition before read entry from BookKeeper to fix BKIncorrectParameterException