Re: [PR] [fix][broker] Fixed the ExtensibleLoadManagerImpl internal system getTopic failure when the leadership changes [pulsar]
heesung-sn commented on PR #21764: URL: https://github.com/apache/pulsar/pull/21764#issuecomment-1869138585 Raised a cherry-pick PR here. https://github.com/apache/pulsar/pull/21801 -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][broker] Fixed the ExtensibleLoadManagerImpl internal system getTopic failure when the leadership changes [pulsar]
Demogorgon314 commented on PR #21764: URL: https://github.com/apache/pulsar/pull/21764#issuecomment-1869006517 @heesung-sn Can you help cherry-pick this PR to `branch-3.1`? -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][broker] Fixed the ExtensibleLoadManagerImpl internal system getTopic failure when the leadership changes [pulsar]
lhotari merged PR #21764: URL: https://github.com/apache/pulsar/pull/21764 -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] [fix][broker] Fixed the ExtensibleLoadManagerImpl internal system getTopic failure when the leadership changes [pulsar]
codecov-commenter commented on PR #21764: URL: https://github.com/apache/pulsar/pull/21764#issuecomment-1864197268 ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/21764?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report Attention: `12 lines` in your changes are missing coverage. Please review. > Comparison is base [(`0ad1867`)](https://app.codecov.io/gh/apache/pulsar/commit/0ad18670263d1331ffa220f36883db8615abac89?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 72.94% compared to head [(`8b5062f`)](https://app.codecov.io/gh/apache/pulsar/pull/21764?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 73.43%. > Report is 4 commits behind head on master. Additional details and impacted files [](https://app.codecov.io/gh/apache/pulsar/pull/21764?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) ```diff @@ Coverage Diff @@ ## master #21764 +/- ## + Coverage 72.94% 73.43% +0.49% - Complexity3257932786 +207 Files 1897 1897 Lines140627 140647 +20 Branches 1548615489 +3 + Hits 102581 103288 +707 + Misses2997429285 -689 - Partials 8072 8074 +2 ``` | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/21764/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [inttests](https://app.codecov.io/gh/apache/pulsar/pull/21764/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.17% <4.54%> (?)` | | | [systests](https://app.codecov.io/gh/apache/pulsar/pull/21764/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.94% <4.54%> (+0.23%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/21764/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.71% <81.81%> (+0.38%)` | :arrow_up: | 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=apache#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/apache/pulsar/pull/21764?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [...ervice/AbstractDispatcherSingleActiveConsumer.java](https://app.codecov.io/gh/apache/pulsar/pull/21764?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Fic3RyYWN0RGlzcGF0Y2hlclNpbmdsZUFjdGl2ZUNvbnN1bWVyLmphdmE=) | `90.55% <100.00%> (+3.24%)` | :arrow_up: | | [...a/org/apache/pulsar/broker/service/Dispatcher.java](https://app.codecov.io/gh/apache/pulsar/pull/21764?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL0Rpc3BhdGNoZXIuamF2YQ==) | `55.55% <100.00%> (+11.80%)` | :arrow_up: | | [...tent/NonPersistentDispatcherMultipleConsumers.java](https://app.codecov.io/gh/apache/pulsar/pull/21764?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL25vbnBlcnNpc3RlbnQvTm9uUGVyc2lzdGVudERpc3BhdGNoZXJNdWx0aXBsZUNvbnN1bWVycy5qYXZh) | `68.67% <100.00%> (+0.38%)` | :arrow_up: | | [...sistent/PersistentDispatcherMultipleConsumers.java](https://app.codecov.io/gh/apache/pulsar/pull/21764?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9zZXJ2aWNlL3BlcnNpc3RlbnQvUGVyc2lzdGVudERp
Re: [PR] [fix][broker] Fixed the ExtensibleLoadManagerImpl internal system getTopic failure when the leadership changes [pulsar]
heesung-sn commented on code in PR #21764:
URL: https://github.com/apache/pulsar/pull/21764#discussion_r1432349248
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/store/TableViewLoadDataStoreImpl.java:
##
@@ -39,7 +39,7 @@ public class TableViewLoadDataStoreImpl implements
LoadDataStore {
private TableView tableView;
-private final Producer producer;
+private Producer producer;
Review Comment:
Yes, we better make them volatile. Updated.
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [fix][broker] Fixed the ExtensibleLoadManagerImpl internal system getTopic failure when the leadership changes [pulsar]
heesung-sn commented on code in PR #21764:
URL: https://github.com/apache/pulsar/pull/21764#discussion_r1432349165
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java:
##
@@ -789,74 +789,70 @@ public static boolean isInternalTopic(String topic) {
@VisibleForTesting
void playLeader() {
-if (role != Leader) {
-log.info("This broker:{} is changing the role from {} to {}",
-pulsar.getLookupServiceAddress(), role, Leader);
-int retry = 0;
-while (true) {
+log.info("This broker:{} is setting the role from {} to {}",
+pulsar.getLookupServiceAddress(), role, Leader);
+int retry = 0;
+while (true) {
Review Comment:
Thanks for sharing this practice. Updated.
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/store/TableViewLoadDataStoreImpl.java:
##
@@ -39,7 +39,7 @@ public class TableViewLoadDataStoreImpl implements
LoadDataStore {
private TableView tableView;
-private final Producer producer;
+private Producer producer;
Review Comment:
Updated
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [fix][broker] Fixed the ExtensibleLoadManagerImpl internal system getTopic failure when the leadership changes [pulsar]
lhotari commented on code in PR #21764:
URL: https://github.com/apache/pulsar/pull/21764#discussion_r1432325288
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/store/TableViewLoadDataStoreImpl.java:
##
@@ -39,7 +39,7 @@ public class TableViewLoadDataStoreImpl implements
LoadDataStore {
private TableView tableView;
-private final Producer producer;
+private Producer producer;
Review Comment:
Nnow that this isn't a final field anymore, is there a need to make it
volatile for thread safety?
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/ExtensibleLoadManagerImpl.java:
##
@@ -789,74 +789,70 @@ public static boolean isInternalTopic(String topic) {
@VisibleForTesting
void playLeader() {
-if (role != Leader) {
-log.info("This broker:{} is changing the role from {} to {}",
-pulsar.getLookupServiceAddress(), role, Leader);
-int retry = 0;
-while (true) {
+log.info("This broker:{} is setting the role from {} to {}",
+pulsar.getLookupServiceAddress(), role, Leader);
+int retry = 0;
+while (true) {
Review Comment:
Just wondering about a possible shutdown scenario. For those cases, it would
be better to terminate loops based on the thread's interrupt status instead of
having `while(true) {` loops.
```
while (!Thread.currentThread.isInterrupted()) {
```
In addition to this, I'd recommend preserving Thread interrupt status
whenever InterruptedException is caught.
```
try {
Thread.sleep(x);
} catch (InterruptedException e) {
// preserve thread's interrupt status
Thread.currentThread().interrupt();
}
```
(one of the references https://stackoverflow.com/a/52623479)
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/store/TableViewLoadDataStoreImpl.java:
##
@@ -39,7 +39,7 @@ public class TableViewLoadDataStoreImpl implements
LoadDataStore {
private TableView tableView;
-private final Producer producer;
+private Producer producer;
Review Comment:
Now that this isn't a final field anymore, is there a need to make it
volatile for thread safety?
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] [fix][broker] Fixed the ExtensibleLoadManagerImpl internal system getTopic failure when the leadership changes [pulsar]
dragosvictor commented on PR #21764: URL: https://github.com/apache/pulsar/pull/21764#issuecomment-1863940217 LGTM! -- 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-unsubscr...@pulsar.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
