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/08/24 04:49:54 UTC
[GitHub] [pulsar] lhotari opened a new pull request, #17252: [fix][broker] Make timestamp fields thread safe by using volatile to prevent visibility issues
lhotari opened a new pull request, #17252:
URL: https://github.com/apache/pulsar/pull/17252
### Motivation
- fixes issue with stats where timestamps might be inconsistent because of visibility issues
- most timestamps already are volatile, and it's better to follow this approach to prevent field update visibility issues
- some visibility issues might only be reproducible on certain hardware platforms (multiple CPU sockets, multiple NUMA nodes, etc.) so one cannot be sure about the behavior unless code is made thread safe
### Modifications
- make fields 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] lhotari commented on a diff in pull request #17252: [fix][broker] Make timestamp fields thread safe by using volatile to prevent visibility issues
Posted by GitBox <gi...@apache.org>.
lhotari commented on code in PR #17252:
URL: https://github.com/apache/pulsar/pull/17252#discussion_r953376245
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java:
##########
@@ -129,7 +129,7 @@ public class Consumer {
private static final double avgPercent = 0.9;
private boolean preciseDispatcherFlowControl;
- private PositionImpl readPositionWhenJoining;
+ private volatile PositionImpl readPositionWhenJoining;
Review Comment:
that's true. the design of PositionImpl is not optimal since it's not an immutable class. However, in this case, the instance that is used is an "effectively immutable" instance.
--
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 #17252: [fix][broker] Make timestamp fields thread safe by using volatile to prevent visibility issues
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17252:
URL: https://github.com/apache/pulsar/pull/17252#issuecomment-1225222348
@lhotari Please provide a correct documentation label for your PR.
Instructions see [Pulsar Documentation Label Guide](https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0).
--
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 merged pull request #17252: [fix][broker] Make timestamp fields thread safe by using volatile to prevent visibility issues
Posted by GitBox <gi...@apache.org>.
lhotari merged PR #17252:
URL: https://github.com/apache/pulsar/pull/17252
--
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 #17252: [fix][broker] Make timestamp fields thread safe by using volatile to prevent visibility issues
Posted by GitBox <gi...@apache.org>.
michaeljmarshall commented on code in PR #17252:
URL: https://github.com/apache/pulsar/pull/17252#discussion_r953364078
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/Consumer.java:
##########
@@ -129,7 +129,7 @@ public class Consumer {
private static final double avgPercent = 0.9;
private boolean preciseDispatcherFlowControl;
- private PositionImpl readPositionWhenJoining;
+ private volatile PositionImpl readPositionWhenJoining;
Review Comment:
The `PositionImpl` class has two mutable `long` fields. Given that it is only ever serialized to a `String`, we might avoid some edge cases by making this object a `volatile String`.
--
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 #17252: [fix][broker] Make timestamp fields thread safe by using volatile to prevent visibility issues
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #17252:
URL: https://github.com/apache/pulsar/pull/17252#issuecomment-1256835651
The pr had no activity for 30 days, mark with Stale label.
--
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