You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "michaeljmarshall (via GitHub)" <gi...@apache.org> on 2023/03/20 18:21:22 UTC
[GitHub] [pulsar] michaeljmarshall opened a new issue, #19866: Verify at least one URL is present for cluster creation
michaeljmarshall opened a new issue, #19866:
URL: https://github.com/apache/pulsar/issues/19866
### Search before asking
- [X] I searched in the [issues](https://github.com/apache/pulsar/issues) and found nothing similar.
### Motivation
With https://github.com/apache/pulsar/pull/19151 and https://github.com/apache/pulsar/pull/19762, we verify that cluster urls are valid. It seems like we could also verify that there is at least one `serviceUrl`/`serviceUrlTls` and at least one `brokerServiceUrl`/`brokerServiceUrlTls` set.
### Solution
Update the following code to add a check that some of those variables are set:
https://github.com/apache/pulsar/blob/e829672b84c15a596574da25205d32ae7b1abf4b/pulsar-common/src/main/java/org/apache/pulsar/common/policies/data/ClusterDataImpl.java#L412-L430
### Alternatives
_No response_
### Anything else?
_No response_
### Are you willing to submit a PR?
- [ ] I'm willing to submit a PR!
--
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.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] niharrathod commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "niharrathod (via GitHub)" <gi...@apache.org>.
niharrathod commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1479780370
@michaeljmarshall
I have raised a [PR](https://github.com/niharrathod/pulsar/pull/2) having fix for this issue on my personal fork. Could you please take a quick look at it?
I need to patch many existing unit test cases. So want confirm before fixing them.
--
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 issue #19866: Verify at least one URL is present for cluster creation
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1478425788
Sure, thanks @niharrathod
--
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] JooHyukKim commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "JooHyukKim (via GitHub)" <gi...@apache.org>.
JooHyukKim commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1575919522
> Perhaps we lift this check to checkPropertiesIfPresent and that should not break existing tests.
May I ask you what you meant by "lift this check", @tisonkun ?
From my perspective, whatever we do, we still need to fix all tests that configures only one of `ServiceUrl` or `BrokerServiceUrl`.
--
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] tisonkun closed issue #19866: Verify at least one URL is present for cluster creation
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun closed issue #19866: Verify at least one URL is present for cluster creation
URL: https://github.com/apache/pulsar/issues/19866
--
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 issue #19866: Verify at least one URL is present for cluster creation
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1551882906
I am not sure what the right solution is. Given that it sounds like there is a large diff, it might be more complicated than it is worth.
--
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] JooHyukKim commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "JooHyukKim (via GitHub)" <gi...@apache.org>.
JooHyukKim commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1556131165
> I'm now suspecting the condition of this issue -
>
> From the source code it seems we actually tolerate only one of `ServiceUrl ` or `BrokerServiceUrl` configured.
>
> Perhaps we lift this check to checkPropertiesIfPresent and that should not break existing tests.
Is that so? I will try to apply the changes and verify later today 👍🏻
--
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 issue #19866: Verify at least one URL is present for cluster creation
Posted by "michaeljmarshall (via GitHub)" <gi...@apache.org>.
michaeljmarshall commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1498527023
Thanks for your work @niharrathod. I didn't realize it would have such wide reaching impact.
--
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] JooHyukKim commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "JooHyukKim (via GitHub)" <gi...@apache.org>.
JooHyukKim commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1582407793
My apologies for the delay, @tisonkun! 🙌 Here is my analysis and proposal in semi-PIP style, referencing https://github.com/apache/pulsar/issues/16691, thought it will be easier to understand. Let me know what you think 👍🏻👍🏻
## Proposal : Warning on failed verifcation
### Analysis : current Implementation
- Done about handling of `BrokerServiceUrl` vs `ServiceUrl`.
- Below are the most obvious places that I could find with the help of IntelliJ IDE.
- Further investigation(searching) shall be done only if we agree necessary.
| Method Location | Behavior | URL |
|-----------------------------------------|----------------------------------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `WebSocketService.createClientInstance` | `BrokerServiceUrl` is used if not blank, otherwise `ServiceUrl` is used. | [Link 1](https://github.com/apache/pulsar/blob/c39f7bb123a995538fc8fa9a54b572c73fee39db/pulsar-websocket/src/main/java/org/apache/pulsar/websocket/WebSocketService.java#L220-L231) |
| `BrokerService.getReplicationClient` | `BrokerServiceUrl` is used if not blank, otherwise `ServiceUrl` is used. | [Link 2](https://github.com/apache/pulsar/blob/c39f7bb123a995538fc8fa9a54b572c73fee39db/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java#L1350-L1351) |
| `NameSpaceService.getNamespaceClient` | `BrokerServiceUrl` is used if not blank, otherwise `ServiceUrl` is used. | [Link 3](https://github.com/apache/pulsar/blob/c39f7bb123a995538fc8fa9a54b572c73fee39db/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java#L1480-L1491) |
| `TopicLookupBase.lookupTopicAsync` | Verifies at least one of `BrokerServiceUrl` and `ServiceUrl` is not blank. | [Link 4](https://github.com/apache/pulsar/blob/aa7decc5b75894e864f3c5962c5cffad255abf25/pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java#L239-L249) |
....with this search result and considering additional checks like using `tlsEnable` are spread around, **throwing an exception on failed URL verification** seems to cost more than what it's worth.
### Proposal & Conculsion
> although, if BrokerServiceUrl takes precedence over ServiceUrl, perhaps a warning is enough. That said, a clear model
> of these options is somehow missing.
I also think a warning is a viable starting option at this point. Like you said, we still do not have a clear model of
these options. Even before clarifying the configuration model in the future, users can already enjoy the benefit from a proper warning message on misconfiguration.
--
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] niharrathod commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "niharrathod (via GitHub)" <gi...@apache.org>.
niharrathod commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1498485660
@michaeljmarshall : FYI
unassigning myself
I tried to fix this and raised PR. In-fact actual change is easy and small, But the same change breaks almost all unit test-cases around broker sub-systems. So, overall fix becomes really huge.
If anyone looking into this issue and wants help, feel free to DM me.
--
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] tisonkun commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1579859518
@JooHyukKim If you take a closer look at https://github.com/apache/pulsar/issues/19866#issuecomment-1556077694, it supports fallback to `serviceUrl` when `brokerServiceUrl` is not configured.
The other use case of `ClientDataImpl` is:
https://github.com/apache/pulsar/blob/aa7decc5b75894e864f3c5962c5cffad255abf25/pulsar-broker/src/main/java/org/apache/pulsar/broker/lookup/TopicLookupBase.java#L239-L245
So...I must say here we have a series of config problems:
* Can `serviceUrl` be a legacy option key of `brokerServiceUrl`?
* Can `serviceUrlTls` be a legacy option key of `brokerServiceUrlTls`?
* What's more vague, `tlsEnable` is deprecated and it seems no options fall back here, but it's dependent by `NamespaceService` logics.
Perhaps we can open a new issue to investigate these issues and see if we can have a clear model.
> we still need to fix all tests that configures only one of ServiceUrl or BrokerServiceUrl.
Theoretically, I agree, while it's already different from the proposal of this issue:
> verify that there is at least one serviceUrl/serviceUrlTls and at least one brokerServiceUrl/brokerServiceUrlTls set.
... although, if `BrokerServiceUrl` takes precedence over `ServiceUrl`, perhaps a warning is enough. That said, a clear model of these options is somehow missing.
--
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 issue #19866: Verify at least one URL is present for cluster creation
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1537267710
The issue 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
[GitHub] [pulsar] tisonkun commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1556077694
https://github.com/apache/pulsar/blob/aa7decc5b75894e864f3c5962c5cffad255abf25/pulsar-broker/src/main/java/org/apache/pulsar/broker/namespace/NamespaceService.java#L1469-L1480
I'm now suspecting the condition of this issue -
From the source code it seems we actually tolerate only one of `ServiceUrl ` or `BrokerServiceUrl` configured.
--
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] JooHyukKim commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "JooHyukKim (via GitHub)" <gi...@apache.org>.
JooHyukKim commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1546630954
That way we can minimize change scope.
--
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] niharrathod commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "niharrathod (via GitHub)" <gi...@apache.org>.
niharrathod commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1478335242
I can take up this. Could you please assign this to me?
--
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
Re: [I] Verify at least one URL is present for cluster creation [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-2171171974
For Pulsar, it's sufficient that one of the 4 urls is present. Pulsar client will use http/https based lookup if the Pulsar binary protocol url is missing.
--
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] JooHyukKim commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "JooHyukKim (via GitHub)" <gi...@apache.org>.
JooHyukKim commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1546630883
@michaeljmarshall Would you consider, starting the verification as an option? Then enable the verification module-by-module
--
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] JooHyukKim commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "JooHyukKim (via GitHub)" <gi...@apache.org>.
JooHyukKim commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1552968147
> I am not sure what the right solution is. Given that it sounds like there is a large diff, it might be more complicated than it is worth.
The required changes being large, it is daunting IMHO. But I am just saying the verification (what this issue is trying to solve) still seem useful, especially when users need to rush --failing fast will help reduce time wasted. 👍🏻
So, taking this plan further, when pushing through, we can choose either of 2 possible scenarios below.
### A. Gradual change.
1. Add new configuration option `checkNeededUrlExist`.
2. With usage `CluterData.builder().checkNeededUrlExist(false).build();`.
3. Value of `checkNeededUrlExist` will be `false` by default.
2. Document that you can create cluster safely by enabling checkNeededUrlExist(true)
3. Document that `checkNeededUrlExist(true)` will be enabled by default at some point.
4. Slowly fix broken tests.
Pros: breaking changes into bits.
Cons: Harder to revert.
### B. All at once
We can pick up where https://github.com/apache/pulsar/pull/19965 was left off.
--
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] tisonkun commented on issue #19866: Verify at least one URL is present for cluster creation
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on issue #19866:
URL: https://github.com/apache/pulsar/issues/19866#issuecomment-1600086479
We have a good start point with @JooHyukKim's #20541. There is not yet a consensus on if we want to apply strict rules on these options. We can see how the warnings hint users.
Closing as no consensus...
If anyone has a proposal for changing the current behavior (I suspect we don't need eagerly), please open a new issue with the details.
--
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