You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "nicoloboschi (via GitHub)" <gi...@apache.org> on 2023/02/14 11:32:14 UTC
[GitHub] [pulsar] nicoloboschi opened a new pull request, #19517: [fix][broker] Copy command fields and fix potential thread-safety in ServerCnx
nicoloboschi opened a new pull request, #19517:
URL: https://github.com/apache/pulsar/pull/19517
### Motivation
In https://github.com/apache/pulsar/pull/19467 we introduced a couple of possible issues about thread-safety of the `BaseCommand` and `ServerCnx` instances.
Original comments from @michaeljmarshall :
- https://github.com/apache/pulsar/pull/19467#discussion_r1101827289
- https://github.com/apache/pulsar/pull/19467#discussion_r1101815870
### Modifications
* Get the auth fields in the same thread of the connection
* Copy partitionList instead of reading it in another thread
### Documentation
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
- [ ] `doc` <!-- Your PR contains doc changes. -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [x] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->
--
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] Technoboy- closed pull request #19517: [fix][broker] Copy command fields and fix potential thread-safety in ServerCnx
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- closed pull request #19517: [fix][broker] Copy command fields and fix potential thread-safety in ServerCnx
URL: https://github.com/apache/pulsar/pull/19517
--
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 a diff in pull request #19517: [fix][broker] Copy command fields and fix potential thread-safety in ServerCnx
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on code in PR #19517:
URL: https://github.com/apache/pulsar/pull/19517#discussion_r1106630819
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2555,7 +2556,7 @@ private CompletableFuture<Boolean> verifyTxnOwnership(TxnID txnID) {
} else {
return CompletableFuture.completedFuture(false);
}
- });
+ }, ctx.executor());
Review Comment:
I requested this change last week because `getAuthenticationData()` was getting a non-volatile variable. However, before this PR was opened, I contributed #19507. @nicoloboschi - we can probably remove this part of the diff since reading the `authenticationData` is now thread safe.
In the long term, it might be worth exploring if we should only read this metadata from the event loop, but for now, these variables are volatile.
--
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] Technoboy- commented on a diff in pull request #19517: [fix][broker] Copy command fields and fix potential thread-safety in ServerCnx
Posted by "Technoboy- (via GitHub)" <gi...@apache.org>.
Technoboy- commented on code in PR #19517:
URL: https://github.com/apache/pulsar/pull/19517#discussion_r1105862805
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -2555,7 +2556,7 @@ private CompletableFuture<Boolean> verifyTxnOwnership(TxnID txnID) {
} else {
return CompletableFuture.completedFuture(false);
}
- });
+ }, ctx.executor());
Review Comment:
Why modify here?
--
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] nicoloboschi merged pull request #19517: [fix][broker] Copy command fields and fix potential thread-safety in ServerCnx
Posted by "nicoloboschi (via GitHub)" <gi...@apache.org>.
nicoloboschi merged PR #19517:
URL: https://github.com/apache/pulsar/pull/19517
--
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