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 2020/06/19 15:45:32 UTC
[GitHub] [pulsar-client-go] cugxuan opened a new pull request #291: [Issue 177] feature: support multi host
cugxuan opened a new pull request #291:
URL: https://github.com/apache/pulsar-client-go/pull/291
Fixes #177
### Motivation
support multi host
### Modifications
1. pulsar/client_impl.go
add hostResolve init
modify rpcClient and lookService init
2. pulsar/consumer_partition.go
modify rpcClient.request,remove the args of function `rpcClient.Request()`
3. pulsar/internal/host_resolve.go
add a struct hostResolve to resolve serviceURL
4. pulsar/internal/host_resolve_test.go
add hostResolve tests
5. pulsar/internal/lookup_service.go
remove serviceURL, add HostResolve
6. pulsar/internal/rpc_client.go
remove serviceURL, add HostResolve, modify request function
7. pulsar/producer_partition.go
modify rpcClient.request,remove the args of function `rpcClient.Request()`
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
### Documentation
- Does this pull request introduce a new feature? yes
- If yes, how is the feature documented? (not applicable / docs / GoDocs / not documented)
- If a feature is not applicable for documentation, explain why?
- If a feature is not documented yet in this PR, please create a followup issue for adding the documentation
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] jiazhai commented on pull request #291: [Issue 177] feature: support multi host
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #291:
URL: https://github.com/apache/pulsar-client-go/pull/291#issuecomment-767261587
@cugxuan Thanks for the contribution, Would you please help rebase with latest master?
also @freeznet Would you please help review this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] cugxuan commented on a change in pull request #291: [Issue 177] feature: support multi host
Posted by GitBox <gi...@apache.org>.
cugxuan commented on a change in pull request #291:
URL: https://github.com/apache/pulsar-client-go/pull/291#discussion_r443084654
##########
File path: pulsar/internal/host_resolve.go
##########
@@ -0,0 +1,107 @@
+package internal
+
+import (
+ "errors"
+ "math/rand"
+ "net/url"
+ "strings"
+ "sync"
+ "time"
+
+ log "github.com/sirupsen/logrus"
+)
+
+// HostResolve is used to implement multihost
+type HostResolve interface {
+ GetServiceURL() string
+ GetHost() (*url.URL, error)
+ GetHostURL() (string, error)
+ ResolveHost() (*url.URL, error)
+}
+
+type hostResolve struct {
+ serviceURL string
+ Host []*url.URL
+ CurrentIndex int
+}
+
+func NewHostResolve(serviceURL string) (HostResolve, error) {
+ h := &hostResolve{
+ serviceURL: serviceURL,
+ }
+
+ // resolve host
+ hosts := strings.Split(h.serviceURL, ",")
+ if len(hosts) == 0 || !checkPrefix(hosts[0]) {
+ return nil, errors.New("invalid service URL")
+ }
+
+ for k := range hosts {
+ if !checkPrefix(hosts[k]) {
Review comment:
Oh, This is something I didn’t think about!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] keithnull commented on a change in pull request #291: [Issue 177] feature: support multi host
Posted by GitBox <gi...@apache.org>.
keithnull commented on a change in pull request #291:
URL: https://github.com/apache/pulsar-client-go/pull/291#discussion_r443083595
##########
File path: pulsar/internal/host_resolve.go
##########
@@ -0,0 +1,107 @@
+package internal
Review comment:
Apache license should be included at the beginning of every file?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] jiazhai commented on pull request #291: [Issue 177] feature: support multi host
Posted by GitBox <gi...@apache.org>.
jiazhai commented on pull request #291:
URL: https://github.com/apache/pulsar-client-go/pull/291#issuecomment-767261587
@cugxuan Thanks for the contribution, Would you please help rebase with latest master?
also @freeznet Would you please help review this 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] cugxuan commented on pull request #291: [Issue 177] feature: support multi host
Posted by GitBox <gi...@apache.org>.
cugxuan commented on pull request #291:
URL: https://github.com/apache/pulsar-client-go/pull/291#issuecomment-767277833
I have updated this PR. This operation brings some conflicts.
If anyone will help to push this PR, I'm glad to spend some time to fix them.
If not, I also could close it. Please give me some info.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] cugxuan closed pull request #291: [Issue 177] feature: support multi host
Posted by GitBox <gi...@apache.org>.
cugxuan closed pull request #291:
URL: https://github.com/apache/pulsar-client-go/pull/291
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] keithnull commented on a change in pull request #291: [Issue 177] feature: support multi host
Posted by GitBox <gi...@apache.org>.
keithnull commented on a change in pull request #291:
URL: https://github.com/apache/pulsar-client-go/pull/291#discussion_r443084172
##########
File path: pulsar/internal/host_resolve.go
##########
@@ -0,0 +1,107 @@
+package internal
+
+import (
+ "errors"
+ "math/rand"
+ "net/url"
+ "strings"
+ "sync"
+ "time"
+
+ log "github.com/sirupsen/logrus"
+)
+
+// HostResolve is used to implement multihost
+type HostResolve interface {
+ GetServiceURL() string
+ GetHost() (*url.URL, error)
+ GetHostURL() (string, error)
+ ResolveHost() (*url.URL, error)
+}
+
+type hostResolve struct {
+ serviceURL string
+ Host []*url.URL
+ CurrentIndex int
+}
+
+func NewHostResolve(serviceURL string) (HostResolve, error) {
+ h := &hostResolve{
+ serviceURL: serviceURL,
+ }
+
+ // resolve host
+ hosts := strings.Split(h.serviceURL, ",")
+ if len(hosts) == 0 || !checkPrefix(hosts[0]) {
+ return nil, errors.New("invalid service URL")
+ }
+
+ for k := range hosts {
+ if !checkPrefix(hosts[k]) {
Review comment:
Maybe `hosts[k - 1]` is a better choice than `hosts[0]`?
For example, `"pulsar://host1,pulsar+ssl://host2,host3"`, in which I think `hosts3` is expected to be `pular+ssl` rather than `pulsar`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar-client-go] cugxuan commented on a change in pull request #291: [Issue 177] feature: support multi host
Posted by GitBox <gi...@apache.org>.
cugxuan commented on a change in pull request #291:
URL: https://github.com/apache/pulsar-client-go/pull/291#discussion_r443084743
##########
File path: pulsar/internal/host_resolve.go
##########
@@ -0,0 +1,107 @@
+package internal
Review comment:
Yes, I am fixing the errors in CI.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org