Skip to content
This repository was archived by the owner on Jan 24, 2024. It is now read-only.

fix direct memory leak bug #542

Merged
BewareMyPower merged 2 commits into from
Jun 11, 2021
Merged

fix direct memory leak bug #542

BewareMyPower merged 2 commits into from
Jun 11, 2021

Conversation

@hangc0276
Copy link
Collaborator

@hangc0276 hangc0276 commented Jun 2, 2021

Fix #442

Modification

  1. Add NPE check for peekOffsetFromEntry and peekBaseOffsetFromEntry
  2. Auto skip fetched exception entry in decode stage to avoid direct memory leak.

@hangc0276 hangc0276 self-assigned this Jun 2, 2021
@BewareMyPower BewareMyPower mentioned this pull request Jun 2, 2021
Closed
@BewareMyPower
Copy link
Collaborator

BewareMyPower commented Jun 9, 2021

Since it could be conflict with #556, I think you can wait until #556 is merged and fix this PR after that.

@jiazhai
Copy link
Contributor

jiazhai commented Jun 9, 2021

@hangc0276 Would you please help update with latest master code?

@hangc0276
Copy link
Collaborator Author

hangc0276 commented Jun 10, 2021

@hangc0276 Would you please help update with latest master code?

OK，I will update the code today.

@hangc0276 hangc0276 force-pushed the chenhang/fix_direct_memory_leak branch from d6b94af to 03de349 Compare June 10, 2021 13:23
@hangc0276 hangc0276 requested a review from BewareMyPower June 10, 2021 13:24
@BewareMyPower
Copy link
Collaborator

BewareMyPower commented Jun 11, 2021

Please fix the spotbugs check

Medium: Exception is caught when Exception is not thrown in io.streamnative.pulsar.handlers.kop.format.PulsarEntryFormatter.lambda$decode$3(ByteBuffer, byte, List, Entry) [io.streamnative.pulsar.handlers.kop.format.PulsarEntryFormatter] At PulsarEntryFormatter.java:[line 228] REC_CATCH_EXCEPTION

And it's better not to use throws Exception directly. Add a new exception type that derived from Exception.

@BewareMyPower
Copy link
Collaborator

BewareMyPower commented Jun 11, 2021

I also noticed OffsetResetTest#testLessThanStartOffset is very easy to fail. If it's convenient, you can mark it as @Ignore in this PR so that we don't need a new PR to fix it. BTW, this test should not be passed now.

@hangc0276
Copy link
Collaborator Author

hangc0276 commented Jun 11, 2021

I also noticed OffsetResetTest#testLessThanStartOffset is very easy to fail. If it's convenient, you can mark it as @Ignore in this PR so that we don't need a new PR to fix it. BTW, this test should not be passed now.

@BewareMyPower I have update the code, PTAL, thanks.

@BewareMyPower BewareMyPower merged commit f357a6f into streamnative:master Jun 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@gaoran10 gaoran10 Awaiting requested review from gaoran10

@aloyszhang aloyszhang Awaiting requested review from aloyszhang

@wuzhanpeng wuzhanpeng Awaiting requested review from wuzhanpeng

2 more reviewers

@jiazhai jiazhai jiazhai approved these changes

@BewareMyPower BewareMyPower BewareMyPower approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@hangc0276 hangc0276

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Encounter corrupted messages after recovered from an OOM

3 participants

@hangc0276 @BewareMyPower @jiazhai