add advertised endpoint to cluster / active fo #192
Conversation
ewoutp
left a comment
ewoutp
left a comment
There was a problem hiding this comment.
Couple of minor issues that needs to be fixed.
Documentation of the new option also need to be added to docs/...
CHANGELOG.md
Outdated
| @@ -1,5 +1,7 @@ | |||
| # Changed from version 0.12.0 to master | |||
|
|
|||
| - Added advertised endpoint to coordinators and active fo | |||
There was a problem hiding this comment.
"Added advertised endpoint to cluster & activefailover deployments."
main.go
Outdated
| f.IntVar(&logRotateFilesToKeep, "log.rotate-files-to-keep", defaultLogRotateFilesToKeep, "Number of files to keep when rotating log files") | ||
| f.DurationVar(&logRotateInterval, "log.rotate-interval", defaultLogRotateInterval, "Time between log rotations (0 disables log rotation)") | ||
|
|
||
| f.StringVar(&advertisedEndpoint, "cluster.advertised-endpoint", "", "An external endpoint for this cluster") |
There was a problem hiding this comment.
Not sure about "this cluster" since it is the coordinator/single.
Also we should talk about the format here, e.g. by adding an example.
There was a problem hiding this comment.
Of course, but it's the cluster's endpoint, right? You mentioned yourself the load balancer scenario.
There was a problem hiding this comment.
it is correct, I just think the comment should be more elaborate.
service/arangod_config_builder.go
Outdated
| ) | ||
| case ServerTypeResilientSingle: | ||
| options = append(options, | ||
| optionPair{"--cluster.my-advertised-endpoint", config.AdvertisedEndpoint}, |
service/arangod_config_builder.go
Outdated
| optionPair{"--foxx.queues", "true"}, | ||
| optionPair{"--server.statistics", "true"}, | ||
| ) | ||
| if config.AdvertisedEndpoint != "" { |
There was a problem hiding this comment.
I'll probably move this block out of the switch and wrap it in an if serverType == ServerTypeCoordinator || serverType == ServerTypeResilientSingle { ... }
This avoids code duplication.
| ArangodPath: arangodPath, | ||
| ArangoSyncPath: arangoSyncPath, | ||
| ArangodJSPath: arangodJSPath, | ||
| AdvertisedEndpoint: advertisedEndpoint, |
There was a problem hiding this comment.
IMO the advertised endpoint should be checked (when non-empty) to make sure it is a valid format.
There was a problem hiding this comment.
Again, also here we had decided to not do any sanity checks. It it does not suffice the endpoint specification, the affected services will complain. Effectively like wrong passthrough options
There was a problem hiding this comment.
Not so sure on the "no sanity checks". If we decided to that, it sounds like a bad decision.
There was a problem hiding this comment.
Now test was added. Problem solved.
| ArangodPath: arangodPath, | ||
| ArangoSyncPath: arangoSyncPath, | ||
| ArangodJSPath: arangodJSPath, | ||
| AdvertisedEndpoint: advertisedEndpoint, |
There was a problem hiding this comment.
Secondly I suggest to allow the use of "http" & "https" schemes in here and automatically transform them to "tcp" & "ssl"
There was a problem hiding this comment.
We had that discussion and decided to the contrary. It had to suffice the enpoint scheme. Keep in mind that this was supposed to not be interpreted in any way and match the endpoint specification and was the reason we decided to not call it advertised-address. No sanity checks no nothing.
There was a problem hiding this comment.
I strongly disagree on this. The starter is supposed to make life easier and having think about "tcp://" or "ssl://" where everyone knows about "http://" and "https://" does not help.
Also we don't have to check anything except replace an "http://" prefix with "tcp://" (and https...)
main.go
Outdated
| "context" | ||
| "fmt" | ||
| "io/ioutil" | ||
| "net/url" |
service/arangod_config_builder.go
Outdated
|
|
||
| // FixupEndpointURLScheme changes endpoint URL schemes used by arangod to ones used by arangodb. | ||
| // E.g. "http://localhost:8529" -> "tcp://localhost:8529" | ||
| func FixupEndpointURLScheme(u string) string { |
There was a problem hiding this comment.
This function does not have to be exported.
service/arangod_config_builder.go
Outdated
| ) | ||
| ) | ||
|
|
||
| // FixupEndpointURLScheme changes endpoint URL schemes used by arangod to ones used by arangodb. |
There was a problem hiding this comment.
'FixupEndpointURLScheme changes endpoint URL schemes used by arangod to ones used by arangodb.' -> 'FixupEndpointURLScheme changes endpoint URL schemes used by Starter to ones used by arangod.'
No description provided.