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 2022/12/19 18:57:29 UTC
[GitHub] [pulsar] michaeljmarshall opened a new pull request, #18987: [fix][txn] Use correct tcId variable to prevent data race
michaeljmarshall opened a new pull request, #18987:
URL: https://github.com/apache/pulsar/pull/18987
### Motivation
The `command` generated protobuf object is not meant to be shared across threads. Instead, we need to copy relevant values into other variables. In the `handleNewTxn` we did not follow this paradigm in the "happy path".
### Modifications
* Replace `command` with `tcId` since the latter is a final variable meant to be published to another thread.
### Verifying this change
This is a trivial change that is already covered by tests.
### Documentation
- [x] `doc-not-needed`
This is an internal change.
### Matching PR in forked repository
PR in forked repository: https://github.com/michaeljmarshall/pulsar/pull/8
--
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] codecov-commenter commented on pull request #18987: [fix][broker] Copy proto command fields into final variables in ServerCnx
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #18987:
URL: https://github.com/apache/pulsar/pull/18987#issuecomment-1358241298
# [Codecov](https://codecov.io/gh/apache/pulsar/pull/18987?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#18987](https://codecov.io/gh/apache/pulsar/pull/18987?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (886796a) into [master](https://codecov.io/gh/apache/pulsar/commit/feb3cb4d7a484a284e06474713870609b220abfc?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (feb3cb4) will **decrease** coverage by `18.91%`.
> The diff coverage is `50.00%`.
[](https://codecov.io/gh/apache/pulsar/pull/18987?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #18987 +/- ##
=============================================
- Coverage 46.35% 27.43% -18.92%
+ Complexity 8939 5130 -3809
=============================================
Files 597 597
Lines 56858 56856 -2
Branches 5905 5906 +1
=============================================
- Hits 26357 15599 -10758
- Misses 27616 38965 +11349
+ Partials 2885 2292 -593
```
| Flag | Coverage Δ | |
|---|---|---|
| unittests | `27.43% <50.00%> (-18.92%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/pulsar/pull/18987?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...va/org/apache/pulsar/broker/service/ServerCnx.java](https://codecov.io/gh/apache/pulsar/pull/18987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL1NlcnZlckNueC5qYXZh) | `31.79% <50.00%> (-17.19%)` | :arrow_down: |
| [...n/java/org/apache/pulsar/client/api/RawReader.java](https://codecov.io/gh/apache/pulsar/pull/18987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9hcGkvUmF3UmVhZGVyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pulsar/broker/admin/v1/Brokers.java](https://codecov.io/gh/apache/pulsar/pull/18987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92MS9Ccm9rZXJzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ava/org/apache/pulsar/broker/admin/v2/Brokers.java](https://codecov.io/gh/apache/pulsar/pull/18987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9Ccm9rZXJzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...va/org/apache/pulsar/broker/admin/v1/Clusters.java](https://codecov.io/gh/apache/pulsar/pull/18987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92MS9DbHVzdGVycy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pulsar/broker/admin/v1/Properties.java](https://codecov.io/gh/apache/pulsar/pull/18987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92MS9Qcm9wZXJ0aWVzLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../org/apache/pulsar/client/impl/RawMessageImpl.java](https://codecov.io/gh/apache/pulsar/pull/18987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NsaWVudC9pbXBsL1Jhd01lc3NhZ2VJbXBsLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [.../apache/pulsar/broker/admin/v2/ResourceGroups.java](https://codecov.io/gh/apache/pulsar/pull/18987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9hZG1pbi92Mi9SZXNvdXJjZUdyb3Vwcy5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...ar/common/naming/PartitionedManagedLedgerInfo.java](https://codecov.io/gh/apache/pulsar/pull/18987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9uYW1pbmcvUGFydGl0aW9uZWRNYW5hZ2VkTGVkZ2VySW5mby5qYXZh) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [...naming/RangeEquallyDivideBundleSplitAlgorithm.java](https://codecov.io/gh/apache/pulsar/pull/18987/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2NvbW1vbi9uYW1pbmcvUmFuZ2VFcXVhbGx5RGl2aWRlQnVuZGxlU3BsaXRBbGdvcml0aG0uamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [166 more](https://codecov.io/gh/apache/pulsar/pull/18987/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
--
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 #18987: [fix][broker] Copy proto command fields into final variables in ServerCnx
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #18987:
URL: https://github.com/apache/pulsar/pull/18987#discussion_r1053650914
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1634,6 +1629,7 @@ protected void handleAck(CommandAck ack) {
final boolean hasRequestId = ack.hasRequestId();
final long requestId = hasRequestId ? ack.getRequestId() : 0;
final long consumerId = ack.getConsumerId();
+ final CommandAck finalAck = getBrokerService().getInterceptor() != null ? new CommandAck().copyFrom(ack) : null;
Review Comment:
You are right, great catch.
--
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 a diff in pull request #18987: [fix][broker] Copy proto command fields into final variables in ServerCnx
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #18987:
URL: https://github.com/apache/pulsar/pull/18987#discussion_r1052969123
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1634,6 +1629,7 @@ protected void handleAck(CommandAck ack) {
final boolean hasRequestId = ack.hasRequestId();
final long requestId = hasRequestId ? ack.getRequestId() : 0;
final long consumerId = ack.getConsumerId();
+ final CommandAck finalAck = getBrokerService().getInterceptor() != null ? new CommandAck().copyFrom(ack) : null;
Review Comment:
`getBrokerService().getInterceptor()` is never null. There's a `BrokerInterceptorDisabled` instance when it's not really used, see https://github.com/apache/pulsar/blob/82237d3684fe506bcb6426b3b23f413422e6e4fb/pulsar-broker/src/main/java/org/apache/pulsar/broker/intercept/BrokerInterceptors.java#L89-L93 .
It would be good to optimize this case so that the copy of the CommandAck isn't done when the interceptor is disabled.
--
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 a diff in pull request #18987: [fix][broker] Copy proto command fields into final variables in ServerCnx
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #18987:
URL: https://github.com/apache/pulsar/pull/18987#discussion_r1053024491
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java:
##########
@@ -1634,6 +1629,7 @@ protected void handleAck(CommandAck ack) {
final boolean hasRequestId = ack.hasRequestId();
final long requestId = hasRequestId ? ack.getRequestId() : 0;
final long consumerId = ack.getConsumerId();
+ final CommandAck finalAck = getBrokerService().getInterceptor() != null ? new CommandAck().copyFrom(ack) : null;
Review Comment:
Handled by #18997
--
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 #18987: [fix][broker] Copy proto command fields into final variables in ServerCnx
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on PR #18987:
URL: https://github.com/apache/pulsar/pull/18987#issuecomment-1360018754
When attempting to cherry-pick this to `branch-2.11`, I ran into conflicts with https://github.com/apache/pulsar/pull/18193. Looking to see if we need to cherry pick that one first.
--
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 merged pull request #18987: [fix][broker] Copy proto command fields into final variables in ServerCnx
Posted by GitBox <gi...@apache.org>.
michaeljmarshall merged PR #18987:
URL: https://github.com/apache/pulsar/pull/18987
--
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 #18987: [fix][broker] Copy proto command fields into final variables in ServerCnx
Posted by GitBox <gi...@apache.org>.
lhotari commented on PR #18987:
URL: https://github.com/apache/pulsar/pull/18987#issuecomment-1359096905
I added #18997 and #18998 as follow ups for this PR. Please review
--
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