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/08/27 07:54:52 UTC
[GitHub] [pulsar] casuallc opened a new pull request #11813: [ISSUE 11796] throw NPE when readEntry
casuallc opened a new pull request #11813:
URL: https://github.com/apache/pulsar/pull/11813
[ISSUE 11796] throw NPE when readEntry
No need doc.
--
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] Shoothzj commented on pull request #11813: [ISSUE 11796] throw NPE when readEntry
Posted by GitBox <gi...@apache.org>.
Shoothzj commented on pull request #11813:
URL: https://github.com/apache/pulsar/pull/11813#issuecomment-925839252
@lhotari code `managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerImpl.java` the callback paas the ctx as null.
But in class `PersistentDispatcherSingleActiveConsumer` `readEntryFailed` method acquire the ctx(Consumer) not be null.
So the code should be
` opReadEntry.readEntriesFailed(new ManagedLedgerException.NoMoreEntriesToReadException("The ceilingKey(K key) method is used to return the " +
"least key greater than or equal to the given key, or null if there is no such key"), ctx);`
Am i right ?
--
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] casuallc commented on a change in pull request #11813: [ISSUE 11796] throw NPE when readEntry
Posted by GitBox <gi...@apache.org>.
casuallc commented on a change in pull request #11813:
URL: https://github.com/apache/pulsar/pull/11813#discussion_r700700625
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
##########
@@ -762,6 +764,9 @@ public void asyncReadEntriesOrWait(int maxEntries, long maxSizeBytes, ReadEntrie
} else {
OpReadEntry op = OpReadEntry.create(this, readPosition, numberOfEntriesToRead, callback,
ctx, maxPosition);
+ if (op.readPosition == null) {
+ return;
Review comment:
Do not need call the method here, if `OpReadEntry` init failed, must ensure `OpReadEntry->readEntriesFailed` will be called, `callback.readEntriesFailed` will be called then.
--
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] casuallc commented on pull request #11813: [ISSUE 11796] throw NPE when readEntry
Posted by GitBox <gi...@apache.org>.
casuallc commented on pull request #11813:
URL: https://github.com/apache/pulsar/pull/11813#issuecomment-916616622
> > good work @casuallc . The changes in this PR seem to be necessary for handling the failure case that is highlighted.
> > btw. There seems to be a similar change #11292 in progress, but that is taking a different approach for solving a similar issue. I'd prefer the approach used in this PR over the changes in 11292 since the solution in this PR is very simple.
>
> @lhotari
> IMHO, there is race condition in this solution.
> If `op.readPosition = cursor.ledger.startReadOperationOnLedger(readPositionRef, op);` is failed in `OpReadEntry.create`, this OpReadEntry is recycled in `readEntriesFailed`.
> So if we check `op.readPosition` after `OpReadEntry.create`, this `OpReadEntry` object could be reused in other threads, and maybe the `readPosition` is properly initialized.
>
> That's why I try to init `readPosition` after each `OpReadEntry.create` method in #11292 , causing the complexity.
@Jason918 Yes, you are right. I should check op but not op.readPosition.
I changed the code, 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] gaoran10 commented on a change in pull request #11813: [ISSUE 11796] throw NPE when readEntry
Posted by GitBox <gi...@apache.org>.
gaoran10 commented on a change in pull request #11813:
URL: https://github.com/apache/pulsar/pull/11813#discussion_r700140082
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
##########
@@ -762,6 +764,9 @@ public void asyncReadEntriesOrWait(int maxEntries, long maxSizeBytes, ReadEntrie
} else {
OpReadEntry op = OpReadEntry.create(this, readPosition, numberOfEntriesToRead, callback,
ctx, maxPosition);
+ if (op.readPosition == null) {
+ return;
Review comment:
Maybe we need to call the method `callback.readEntriesFailed` to make sure the caller gets a result.
--
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 a change in pull request #11813: [ISSUE 11796] throw NPE when readEntry
Posted by GitBox <gi...@apache.org>.
315157973 commented on a change in pull request #11813:
URL: https://github.com/apache/pulsar/pull/11813#discussion_r700726597
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
##########
@@ -622,9 +622,11 @@ public void asyncReadEntries(int numberOfEntriesToRead, long maxSizeBytes, ReadE
int numOfEntriesToRead = applyMaxSizeCap(numberOfEntriesToRead, maxSizeBytes);
- PENDING_READ_OPS_UPDATER.incrementAndGet(this);
OpReadEntry op = OpReadEntry.create(this, readPosition, numOfEntriesToRead, callback, ctx, maxPosition);
- ledger.asyncReadEntries(op);
+ if (op.readPosition != null) {
Review comment:
op is not recycle if op.readPosition == null
By the way, could you add some unit test?
--
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] casuallc commented on pull request #11813: [ISSUE 11796] throw NPE when readEntry
Posted by GitBox <gi...@apache.org>.
casuallc commented on pull request #11813:
URL: https://github.com/apache/pulsar/pull/11813#issuecomment-945261905
> @casuallc It looks like your branch for this PR is gone and this PR cannot be re-opened. I closed it with the intention to trigger a rebuild. Rebuilding on CI was disabled and now I can see that the reason was that the original PR branch has been deleted.
>
> @casuallc Would you mind creating a new PR with the same changes?
#12396
--
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] casuallc commented on a change in pull request #11813: [ISSUE 11796] throw NPE when readEntry
Posted by GitBox <gi...@apache.org>.
casuallc commented on a change in pull request #11813:
URL: https://github.com/apache/pulsar/pull/11813#discussion_r700732109
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
##########
@@ -622,9 +622,11 @@ public void asyncReadEntries(int numberOfEntriesToRead, long maxSizeBytes, ReadE
int numOfEntriesToRead = applyMaxSizeCap(numberOfEntriesToRead, maxSizeBytes);
- PENDING_READ_OPS_UPDATER.incrementAndGet(this);
OpReadEntry op = OpReadEntry.create(this, readPosition, numOfEntriesToRead, callback, ctx, maxPosition);
- ledger.asyncReadEntries(op);
+ if (op.readPosition != null) {
Review comment:
op recycled when the method `OpReadEntry->readEntriesFailed` called.
At present, op init failed only if `op.readPosition = cursor.ledger.startReadOperationOnLedger(readPositionRef, op);` called.
I do not know how to test this case ...
--
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] casuallc commented on a change in pull request #11813: [ISSUE 11796] throw NPE when readEntry
Posted by GitBox <gi...@apache.org>.
casuallc commented on a change in pull request #11813:
URL: https://github.com/apache/pulsar/pull/11813#discussion_r700700625
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
##########
@@ -762,6 +764,9 @@ public void asyncReadEntriesOrWait(int maxEntries, long maxSizeBytes, ReadEntrie
} else {
OpReadEntry op = OpReadEntry.create(this, readPosition, numberOfEntriesToRead, callback,
ctx, maxPosition);
+ if (op.readPosition == null) {
+ return;
Review comment:
Neet not call the method here, if `OpReadEntry` init failed, must ensure `OpReadEntry->readEntriesFailed` will be called, `callback.readEntriesFailed` will be called then.
--
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 #11813: [ISSUE 11796] throw NPE when readEntry
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #11813:
URL: https://github.com/apache/pulsar/pull/11813#issuecomment-914976154
good work @casuallc . The changes in this PR seem to be necessary for handling the failure case that is highlighted.
btw. There seems to be a similar change #11292 in progress, but that is taking a different approach for solving a similar issue. I'd prefer the approach used in this PR over the changes in 11292 since the solution in this PR is very 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] Jason918 commented on pull request #11813: [ISSUE 11796] throw NPE when readEntry
Posted by GitBox <gi...@apache.org>.
Jason918 commented on pull request #11813:
URL: https://github.com/apache/pulsar/pull/11813#issuecomment-916570881
> good work @casuallc . The changes in this PR seem to be necessary for handling the failure case that is highlighted.
>
> btw. There seems to be a similar change #11292 in progress, but that is taking a different approach for solving a similar issue. I'd prefer the approach used in this PR over the changes in 11292 since the solution in this PR is very simple.
@lhotari
IMHO, there is race condition in this solution.
If `op.readPosition = cursor.ledger.startReadOperationOnLedger(readPositionRef, op);` is failed in `OpReadEntry.create`, this OpReadEntry is recycled in `readEntriesFailed`.
So if we check `op.readPosition` after `OpReadEntry.create`, this `OpReadEntry` object could be reused in other threads, and maybe the `readPosition` is properly initialized.
That's why I try to init `readPosition` after each `OpReadEntry.create` method in #11292 , causing the complexity.
--
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