Re: [PR] [fix][broker] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
BewareMyPower merged PR #23119: URL: https://github.com/apache/pulsar/pull/23119 -- 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] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
Demogorgon314 commented on code in PR #23119:
URL: https://github.com/apache/pulsar/pull/23119#discussion_r1703810088
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##
@@ -1763,6 +1764,9 @@ public void listen(StateChangeListener listener) {
@Override
public Set> getOwnershipEntrySet()
{
+if (tableview == null) {
Review Comment:
I have updated the check, and it will throw an exception.
--
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] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
lhotari commented on code in PR #23119:
URL: https://github.com/apache/pulsar/pull/23119#discussion_r1703755707
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##
@@ -1763,6 +1764,9 @@ public void listen(StateChangeListener listener) {
@Override
public Set> getOwnershipEntrySet()
{
+if (tableview == null) {
Review Comment:
@Demogorgon314 @heesung-sn is there a resolution to this discussion?
--
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] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
codecov-commenter commented on PR #23119: URL: https://github.com/apache/pulsar/pull/23119#issuecomment-2268476248 ## [Codecov](https://app.codecov.io/gh/apache/pulsar/pull/23119?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report All modified and coverable lines are covered by tests :white_check_mark: > Project coverage is 73.46%. Comparing base [(`bbc6224`)](https://app.codecov.io/gh/apache/pulsar/commit/bbc62245c5ddba1de4b1e7cee4ab49334bc36277?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`c9e1ea3`)](https://app.codecov.io/gh/apache/pulsar/commit/c9e1ea31351aab69b37c46369e409298dca610c8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache). > Report is 491 commits behind head on master. Additional details and impacted files [](https://app.codecov.io/gh/apache/pulsar/pull/23119?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) ```diff @@ Coverage Diff @@ ## master #23119 +/- ## - Coverage 73.57% 73.46% -0.11% - Complexity3262433544 +920 Files 1877 1919 +42 Lines139502 144125+4623 Branches 1529915748 +449 + Hits 102638 105886+3248 - Misses2890830118+1210 - Partials 7956 8121 +165 ``` | [Flag](https://app.codecov.io/gh/apache/pulsar/pull/23119/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/23119/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.60% <25.00%> (+3.01%)` | :arrow_up: | | [systests](https://app.codecov.io/gh/apache/pulsar/pull/23119/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `24.71% <25.00%> (+0.38%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/apache/pulsar/pull/23119/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `72.54% <100.00%> (-0.31%)` | :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=apache#carryforward-flags-in-the-pull-request-comment) to find out more. | [Files](https://app.codecov.io/gh/apache/pulsar/pull/23119?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | |---|---|---| | [...xtensions/channel/ServiceUnitStateChannelImpl.java](https://app.codecov.io/gh/apache/pulsar/pull/23119?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Floadbalance%2Fextensions%2Fchannel%2FServiceUnitStateChannelImpl.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9sb2FkYmFsYW5jZS9leHRlbnNpb25zL2NoYW5uZWwvU2VydmljZVVuaXRTdGF0ZUNoYW5uZWxJbXBsLmphdmE=) | `86.42% <100.00%> (+1.11%)` | :arrow_up: | | [...ache/pulsar/broker/namespace/NamespaceService.java](https://app.codecov.io/gh/apache/pulsar/pull/23119?src=pr&el=tree&filepath=pulsar-broker%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fpulsar%2Fbroker%2Fnamespace%2FNamespaceService.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cHVsc2FyLWJyb2tlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcHVsc2FyL2Jyb2tlci9uYW1lc3BhY2UvTmFtZXNwYWNlU2VydmljZS5qYXZh) | `74.21% <100.00%> (+1.97%)` | :arrow_up: | ... and [522 files with indirect coverage changes](https://app.codecov.io/gh/apache/pulsar/pull/23119/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use
Re: [PR] [fix][broker] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
Demogorgon314 commented on code in PR #23119:
URL: https://github.com/apache/pulsar/pull/23119#discussion_r1703493282
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##
@@ -1763,6 +1764,9 @@ public void listen(StateChangeListener listener) {
@Override
public Set> getOwnershipEntrySet()
{
+if (tableview == null) {
Review Comment:
Oh, I see, we should use `pulsar.runWhenReadyForIncomingRequests` for the
protocol handler side to ensure the channel started.
https://github.com/apache/pulsar/pull/22977
--
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] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
Demogorgon314 commented on code in PR #23119:
URL: https://github.com/apache/pulsar/pull/23119#discussion_r1703508717
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##
@@ -1342,7 +1342,11 @@ public void
addNamespaceBundleOwnershipListener(NamespaceBundleOwnershipListener
}
}
pulsar.runWhenReadyForIncomingRequests(() -> {
-getOwnedServiceUnits().forEach(bundle ->
notifyNamespaceBundleOwnershipListener(bundle, listeners));
+try {
+getOwnedServiceUnits().forEach(bundle ->
notifyNamespaceBundleOwnershipListener(bundle, listeners));
Review Comment:
The `notifyNamespaceBundleOwnershipListener ` method already caught
exceptions. See:
https://github.com/apache/pulsar/blob/76f16e811beb4f48fb2ae5c46558b74d333c7d60/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java#L1362-L1368
--
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] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
BewareMyPower commented on code in PR #23119:
URL: https://github.com/apache/pulsar/pull/23119#discussion_r1703495255
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java:
##
@@ -1342,7 +1342,11 @@ public void
addNamespaceBundleOwnershipListener(NamespaceBundleOwnershipListener
}
}
pulsar.runWhenReadyForIncomingRequests(() -> {
-getOwnedServiceUnits().forEach(bundle ->
notifyNamespaceBundleOwnershipListener(bundle, listeners));
+try {
+getOwnedServiceUnits().forEach(bundle ->
notifyNamespaceBundleOwnershipListener(bundle, listeners));
Review Comment:
I think we should process the possible exception from
`notifyNamespaceBundleOwnershipListener` as well. Currently, if one listener
fails, the following listeners will be skipped. We'd better catch the
`Throwable` for `notifyNamespaceBundleOwnershipListener` as well.
--
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] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
Demogorgon314 commented on code in PR #23119:
URL: https://github.com/apache/pulsar/pull/23119#discussion_r1703493282
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##
@@ -1763,6 +1764,9 @@ public void listen(StateChangeListener listener) {
@Override
public Set> getOwnershipEntrySet()
{
+if (tableview == null) {
Review Comment:
Oh, I see, we should use `pulsar.runWhenReadyForIncomingRequests` for the
protocol handler side to ensure the channel started.
--
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] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
heesung-sn commented on code in PR #23119:
URL: https://github.com/apache/pulsar/pull/23119#discussion_r1703466855
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##
@@ -1763,6 +1764,9 @@ public void listen(StateChangeListener listener) {
@Override
public Set> getOwnershipEntrySet()
{
+if (tableview == null) {
Review Comment:
```
tableview = pulsar.getClient().newTableViewBuilder(schema)
.topic(TOPIC)
.loadConf(Map.of(
"topicCompactionStrategyClassName",
ServiceUnitStateCompactionStrategy.class.getName()))
.create();
tableview.listen((key, value) -> handle(key, value));
```
plz note that this table view only reacts on the new messages after init.
This is because we don't want to make this broker react on the old messages
whose ownership have been already established.
--
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] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
Demogorgon314 commented on code in PR #23119:
URL: https://github.com/apache/pulsar/pull/23119#discussion_r1702714440
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##
@@ -1763,6 +1764,9 @@ public void listen(StateChangeListener listener) {
@Override
public Set> getOwnershipEntrySet()
{
+if (tableview == null) {
Review Comment:
It should be ok to return an empty set here, for `OwnershipListener`, after
the tableview starts, it will notify the listener.
--
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] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
heesung-sn commented on code in PR #23119:
URL: https://github.com/apache/pulsar/pull/23119#discussion_r1702016168
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##
@@ -1763,6 +1764,9 @@ public void listen(StateChangeListener listener) {
@Override
public Set> getOwnershipEntrySet()
{
+if (tableview == null) {
Review Comment:
In fact, it might have some ownerships when the tableview gets initialized.
Is it safe to return this inconsistent view too early?
--
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] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
Demogorgon314 commented on code in PR #23119:
URL: https://github.com/apache/pulsar/pull/23119#discussion_r1701968576
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##
@@ -1763,6 +1764,9 @@ public void listen(StateChangeListener listener) {
@Override
public Set> getOwnershipEntrySet()
{
+if (tableview == null) {
Review Comment:
If the tableview is null, it means that this broker has no ownership, so we
can return an empty set.
--
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] Handle the case when `getOwnedServiceUnits` fails gracefully [pulsar]
heesung-sn commented on code in PR #23119:
URL: https://github.com/apache/pulsar/pull/23119#discussion_r1701904348
##
pulsar-broker/src/main/java/org/apache/pulsar/broker/loadbalance/extensions/channel/ServiceUnitStateChannelImpl.java:
##
@@ -1763,6 +1764,9 @@ public void listen(StateChangeListener listener) {
@Override
public Set> getOwnershipEntrySet()
{
+if (tableview == null) {
Review Comment:
Shouldn't we throw not initialized exception?
--
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
