Skip to content

[Issue:5669] Fix the ledgerID not found cause NPE - #5809

Merged
wolfstudy merged 4 commits into
apache:masterfrom
wolfstudy:xiaolong/legers_NPE
Dec 23, 2019
Merged

[Issue:5669] Fix the ledgerID not found cause NPE#5809
wolfstudy merged 4 commits into
apache:masterfrom
wolfstudy:xiaolong/legers_NPE

Conversation

@wolfstudy

@wolfstudy wolfstudy commented Dec 6, 2019

Copy link
Copy Markdown
Member

Signed-off-by: xiaolong.ran rxl@apache.org

Fixes #5669

Motivation

when ledgers.ceilingKey return a NULL value, we need to process the ledgerId is NUll case.

Modifications

  • change long ledgerId to Long ledgerId and check if ledgerId is NULL.

@wolfstudy
[Issue:5669] Fix the ledgerID not found cause NPE
65cdb4d 
Signed-off-by: xiaolong.ran <rxl@apache.org>
@wolfstudy wolfstudy self-assigned this Dec 6, 2019
sijie
sijie requested changes Dec 7, 2019
View reviewed changes
Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java Outdated
@jiazhai

jiazhai commented Dec 8, 2019

Copy link
Copy Markdown
Member

@wolfstudy The way to fix it is right. Please help resolve sijie's comments

@wolfstudy
fix comments
16ba1dc 
Signed-off-by: xiaolong.ran <rxl@apache.org>
@wolfstudy
replace ReadEntriesCallback with OpReadEntry
46a0d4a 
Signed-off-by: xiaolong.ran <rxl@apache.org>
@fangpengcheng95

fangpengcheng95 commented Dec 11, 2019

Copy link
Copy Markdown
Contributor

run integration tests

@wolfstudy

wolfstudy commented Dec 12, 2019

Copy link
Copy Markdown
Member Author

@sijie @codelipenghui PTAL

@wolfstudy

wolfstudy commented Dec 12, 2019

Copy link
Copy Markdown
Member Author

run integration tests

@wolfstudy

wolfstudy commented Dec 18, 2019

Copy link
Copy Markdown
Member Author

run integration tests
run java8 tests

@wolfstudy

wolfstudy commented Dec 18, 2019

Copy link
Copy Markdown
Member Author

ping @sijie PTAL

@wolfstudy

wolfstudy commented Dec 18, 2019

Copy link
Copy Markdown
Member Author

run integration tests
run java8 tests

@sijie

sijie commented Dec 20, 2019

Copy link
Copy Markdown
Member

run java8 tests
run integration tests

@sijie sijie added area/broker component/storage type/bug The PR fixed a bug or issue reported a bug labels Dec 20, 2019
@sijie sijie added this to the 2.5.0 milestone Dec 20, 2019
@wolfstudy

wolfstudy commented Dec 20, 2019

Copy link
Copy Markdown
Member Author

org.apache.pulsar.broker.admin.AdminApiOffloadTest.testOffloadV1
org.apache.pulsar.broker.admin.AdminApiOffloadTest.testOffloadV2

run java8 tests
run integration tests

@wolfstudy

wolfstudy commented Dec 21, 2019

Copy link
Copy Markdown
Member Author

2019-12-20\T\13:39:01.339 [ERROR] testPerNamespaceStats(org.apache.pulsar.broker.stats.PrometheusMetricsTest) Time elapsed: 0.081 s <<< FAILURE!
java.lang.AssertionError: expected [true] but found [false]

run java8 tests

@wolfstudy

wolfstudy commented Dec 21, 2019

Copy link
Copy Markdown
Member Author

run java8 tests

1 similar comment
@wolfstudy

wolfstudy commented Dec 23, 2019

Copy link
Copy Markdown
Member Author

run java8 tests

@wolfstudy

wolfstudy commented Dec 23, 2019

Copy link
Copy Markdown
Member Author

2019-12-23\T\02:35:31.273 [ERROR] Tests run: 17, Failures: 1, Errors: 0, Skipped: 14, Time elapsed: 4.759 s <<< FAILURE! - in org.apache.pulsar.client.impl.MessageIdTest
2019-12-23\T\02:35:31.274 [ERROR] setup(org.apache.pulsar.client.impl.MessageIdTest) Time elapsed: 1.369 s <<< FAILURE!
org.apache.pulsar.broker.PulsarServerException: java.io.IOException: Failed to bind Pulsar broker on /0.0.0.0:17777
at org.apache.pulsar.broker.PulsarService.start(PulsarService.java:518)
at org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.startBroker(MockedPulsarServiceBaseTest.java:219)
at org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.startBroker(MockedPulsarServiceBaseTest.java:208)
at org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.init(MockedPulsarServiceBaseTest.java:152)
at org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest.internalSetup(MockedPulsarServiceBaseTest.java:110)
at org.apache.pulsar.broker.service.BrokerTestBase.baseSetup(BrokerTestBase.java:40)
at org.apache.pulsar.client.impl.MessageIdTest.setup(MessageIdTest.java:71)

run java8 tests

@wolfstudy

wolfstudy commented Dec 23, 2019

Copy link
Copy Markdown
Member Author

run java8 tests

@wolfstudy

wolfstudy commented Dec 23, 2019

Copy link
Copy Markdown
Member Author

run java8 tests

@wolfstudy

wolfstudy commented Dec 23, 2019

Copy link
Copy Markdown
Member Author

2019-12-23\T\09:16:57.237 [ERROR] Tests run: 24, Failures: 1, Errors: 0, Skipped: 20, Time elapsed: 1,575.194 s <<< FAILURE! - in org.apache.pulsar.broker.loadbalance.SimpleLoadManagerImplTest
2019-12-23\T\09:16:57.238 [ERROR] setup(org.apache.pulsar.broker.loadbalance.SimpleLoadManagerImplTest) Time elapsed: 443.242 s <<< FAILURE!
java.io.IOException: Couldn't connect to zookeeper server
at org.apache.pulsar.zookeeper.LocalBookkeeperEnsemble$ZKConnectionWatcher.waitForConnection(LocalBookkeeperEnsemble.java:466)
at org.apache.pulsar.zookeeper.LocalBookkeeperEnsemble.initializeZookeper(LocalBookkeeperEnsemble.java:209)
at org.apache.pulsar.zookeeper.LocalBookkeeperEnsemble.start(LocalBookkeeperEnsemble.java:375)
at org.apache.pulsar.zookeeper.LocalBookkeeperEnsemble.start(LocalBookkeeperEnsemble.java:384)
at org.apache.pulsar.broker.loadbalance.SimpleLoadManagerImplTest.setup(SimpleLoadManagerImplTest.java:128)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)

run java8 tests

@wolfstudy
wolfstudy merged commit c635d08 into apache:master Dec 23, 2019
@wolfstudy
wolfstudy deleted the xiaolong/legers_NPE branch December 23, 2019 12:54
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
@wolfstudy @huangdx0726
[Issue:5669] Fix the ledgerID not found cause NPE (apache#5809)
137662b 
## Motivation
when ledgers.ceilingKey return a NULL value, we need to process the ledgerId is NUll case.

## Modifications
change long ledgerId to Long ledgerId and check if ledgerId is NULL.
horizonzy
horizonzy reviewed Apr 10, 2022
View reviewed changes
Comment thread managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java
PositionImpl startReadOperationOnLedger(PositionImpl position) {
long ledgerId = ledgers.ceilingKey(position.getLedgerId());
PositionImpl startReadOperationOnLedger(PositionImpl position, OpReadEntry opReadEntry) {
Long ledgerId = ledgers.ceilingKey(position.getLedgerId());

@horizonzy horizonzy Apr 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change maybe not fix npe, it will cause new npe problem.
see this pr line_1800. if ledgerId is null, unbox will cause npe. And opReadEntry callback didn't put ctx, the callback process need use it, npe again.

horizonzy added a commit to horizonzy/pulsar that referenced this pull request Apr 10, 2022
@horizonzy
patch apache#5809: Fix the ledgerID not found cause NPE.
4956976
@horizonzy horizonzy mentioned this pull request Apr 10, 2022
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@horizonzy horizonzy horizonzy left review comments
@sijie sijie sijie approved these changes
@codelipenghui codelipenghui codelipenghui approved these changes
@merlimat merlimat Awaiting requested review from merlimat
@jiazhai jiazhai Awaiting requested review from jiazhai

Assignees

@wolfstudy wolfstudy

Labels

area/broker type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Milestone

2.5.0

Development

Successfully merging this pull request may close these issues.

Pulsar Broker restart fail owing to create functions/assignments topic fail

6 participants

@wolfstudy @jiazhai @fangpengcheng95 @sijie @codelipenghui @horizonzy