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 2021/07/13 03:26:36 UTC
[GitHub] [pulsar] Jason918 opened a new pull request #11292: fix issue-11282
Jason918 opened a new pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292
<!--
### Contribution Checklist
- Name the pull request in the form "[Issue XYZ][component] Title of the pull request", where *XYZ* should be replaced by the actual issue number.
Skip *Issue XYZ* if there is no associated github issue for this pull request.
Skip *component* if you are unsure about which is the best component. E.g. `[docs] Fix typo in produce method`.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
**(The sections below can be removed for hotfixes of typos)**
-->
*(If this PR fixes a github issue, please add `Fixes #<xyz>`.)*
Fixes #<xyz>
*(or if this PR is one task of a github issue, please add `Master Issue: #<xyz>` to link to the master issue.)*
Master Issue: #<xyz>
### Motivation
*Explain here the context, and why you're making that change. What is the problem you're trying to solve.*
### Modifications
*Describe the modifications you've done.*
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API: (yes / no)
- The schema: (yes / no / don't know)
- The default values of configurations: (yes / no)
- The wire protocol: (yes / no)
- The rest endpoints: (yes / no)
- The admin cli options: (yes / no)
- Anything that affects deployment: (yes / no / don't know)
### Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
- If a feature is not applicable for documentation, explain why?
- If a feature is not documented yet in this PR, please create a followup issue for adding the documentation
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-977725196
> @Jason918 seems this is a bug fix (no need to update docs)?
Yes.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] 315157973 commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
315157973 commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-956102318
@lhotari @codelipenghui @hangc0276 PTAL
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 closed pull request #11292: [Issue-11282] Fix NPE in OpReadEntry
Posted by GitBox <gi...@apache.org>.
Jason918 closed pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-974749109
@Jason918:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #11292: [Issue-11282] Fix NPE in OpReadEntry
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-1020879627
Close by duplication.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-878809101
> I think this won't work. When readEntriesFailed is called during initiate readPosition, the property readPosition was not assigned yet, and readEntriesFailed has a chance to call readPosition.getLedgerId()，which will cause another NPE.
>
> Further more, I guess the callbacks should be concerned also.
It's NoMoreEntriesToReadException in `startReadOperationOnLedger` right? So it's not running into the if branch for `readPosition.getLedgerId()`.
And the `callback` is also assigned before the assignment of `readPosition`.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on pull request #11292: [Issue-11282] Fix NPE in OpReadEntry
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-1036616669
Removing release label and milestone since the PR is closed. Please re-add if we need to open the PR.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Anonymitaet commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
Anonymitaet commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-977663197
@Jason918 seems this is a bug fix (no need to update docs)?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-914977596
There seems to be an alternative approach to solve the NPE in PR #11813 . I currently prefer that change since it's fairly simple.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] littleorca commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
littleorca commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-878879720
> > I think this won't work. When readEntriesFailed is called during initiate readPosition, the property readPosition was not assigned yet, and readEntriesFailed has a chance to call readPosition.getLedgerId()，which will cause another NPE.
> > Further more, I guess the callbacks should be concerned also.
>
> It's NoMoreEntriesToReadException in `startReadOperationOnLedger` right? So it's not running into the if branch for `readPosition.getLedgerId()`.
>
> And the `callback` is also assigned before the assignment of `readPosition`.
Seems you are right. I've tried to reorder statements in create() method and ended up in failure, I probably messed up some details about the failure.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-895962872
FYI, there are some thread safety issues in OpReadEntry . PR #11387 attempts to fix some of the thread safety issues.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] hangc0276 commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
hangc0276 commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-896684002
move to 2.8.2.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-974749109
@Jason918:Thanks for your contribution. For this PR, do we need to update docs?
(The [PR template contains info about doc](https://github.com/apache/pulsar/blob/master/.github/PULL_REQUEST_TEMPLATE.md#documentation), which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #11292: [Issue-11282] Fix NPE in OpReadEntry
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-1020879627
Close by duplication.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 closed pull request #11292: [Issue-11282] Fix NPE in OpReadEntry
Posted by GitBox <gi...@apache.org>.
Jason918 closed pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-916571893
> There seems to be an alternative approach to solve the NPE in PR #11813 . I currently prefer that change since it's fairly simple.
There maybe some race conditions in PR #11813, explained the details in that PR, PTAL. :)
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] github-actions[bot] commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-977725239
@Jason918:Thanks for providing doc info!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on pull request #11292: fix issue-11282
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #11292:
URL: https://github.com/apache/pulsar/pull/11292#issuecomment-878836257
@littleorca After deep looking into this case, I found that more serious issue is that, when ManagedLedgerException.NoMoreEntriesToReadException occurs in `OpReadEntry.create`, this OpReadEntry object is actually recycled when create method finished. Any usage of this object would lead to some unexpected behavior.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org