Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add flags to allow changing ports for HotROD services #951

Merged

Conversation

cboornaz17
Copy link
Contributor

Which problem is this PR solving?

Resolves #948

Short description of the changes

Added four flags, one for each service (driver "-d", route "-r", customer "-c", and frontend "-f") so that a user can specify on the command line which port they want each service to be run on. The ports get added to the services/config file and are checked before starting each server. The default port values are still used if the user does not add these flags.

Signed-off-by: Corey Boornazian coreyb220@gmail.com

@codecov
Copy link

codecov bot commented Jul 24, 2018

Codecov Report

Merging #951 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #951   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files         139    139           
  Lines        6399   6399           
=====================================
  Hits         6399   6399

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70c26db...424b30f. Read the comment docs.

fixCustomerPort int
fixDriverPort int
fixFrontendPort int
fixRoutePort int
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"fix" prefix was used for variables about because they can be used to "fix" performance problems instead of changing source code as was done in the blog post. For port numbers there's no "fixing".

@@ -35,7 +36,7 @@ var customerCmd = &cobra.Command{
zapLogger := logger.With(zap.String("service", "customer"))
logger := log.NewFactory(zapLogger)
server := customer.NewServer(
net.JoinHostPort(customerOptions.serverInterface, strconv.Itoa(customerOptions.serverPort)),
net.JoinHostPort(customerOptions.serverInterface, strconv.Itoa(config.CustomerPort)),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about customerOptions.serverPort? If it's no longer used anywhere we should delete it from the struct.

RootCmd.PersistentFlags().IntVarP(&fixCustomerPort, "set-customer-service-port", "c", 8081, "Set port for customer service")
RootCmd.PersistentFlags().IntVarP(&fixDriverPort, "set-driver-service-port", "d", 8082, "Set port for driver service")
RootCmd.PersistentFlags().IntVarP(&fixFrontendPort, "set-frontend-service-port", "f", 8080, "Set port for frontend service")
RootCmd.PersistentFlags().IntVarP(&fixRoutePort, "set-route-service-port", "r", 8083, "Set port for routing service")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for set- prefix.
No need for "Set " in the description.

@@ -90,6 +102,26 @@ func onInitialize() {
logger.Info("fix: overriding route worker pool size", zap.Int("old", config.RouteWorkerPoolSize), zap.Int("new", fixRouteWorkerPoolSize))
config.RouteWorkerPoolSize = fixRouteWorkerPoolSize
}

if config.CustomerPort != fixCustomerPort {
logger.Info("fix: changing customer service port", zap.Int("old", config.CustomerPort), zap.Int("new", fixCustomerPort))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for "fix: " prefix

@yurishkuro
Copy link
Member

Please make sure all commits are signed.

you can

git commit --amend -s
git push --force

If you already missed signing of an earlier commit, please squash them.

@yurishkuro
Copy link
Member

Thanks for the PR

@cboornaz17
Copy link
Contributor Author

Wasn't able to squash them properly and thus made more of a mess in this PR thread by rebasing commits. The new commits are at least signed off. My apologies for the inconvenience.

@yurishkuro
Copy link
Member

yurishkuro commented Jul 24, 2018

fyi, I added some tips on how to squash (the easiest way I know of):
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md#missing-sign-offs

@yurishkuro yurishkuro changed the title Add flags so user can choose ports for services Add flags to allow changing ports for HotROD services Jul 24, 2018
// Port settings, can be overwritten from command line

// Customer Service Port
CustomerPort = 8081
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: why do we need these in the config? The variables that hold the parsed flag values are already in the same package as the commands for services, so we can access the variables directly.

@yurishkuro
Copy link
Member

I am curious about the other command-local flags like the "interface":

	driverCmd.Flags().StringVarP(&driverOptions.serverInterface, "bind", "", "0.0.0.0", "interface to which the driver server will bind")

Not quite sure why they were added as flags (here #694, cc @gbaufake), because they suffer from the same problem as the port numbers, i.e. if the flag is local to the command then the other services won't be able to find the given service if it runs on a non-default IP/interface.

@cboornaz17
Copy link
Contributor Author

I did spend some time trying to directly access the flags provided, but I found that when searching for the proper flags in the customer.go init() function, as well as in the customerCmd function, the flags are not yet parsed. In other words, when I called RootCmd.Flags().Visit(), and set up a simple function to print any flags that are provided, nothing was printed. I read on the cobra docs that the flags are not passed in until the command is executed, which is why I think we see this behavior.

@yurishkuro
Copy link
Member

I understand that, but the value is used inside each command's execute method

net.JoinHostPort(customerOptions.serverInterface, strconv.Itoa(config.CustomerPort)),

so instead of reading from config you should be able to read directly from customerPort private var defined in root.go

@cboornaz17
Copy link
Contributor Author

I can clean up the usage of command-local flags for server interface as well.

I think it may be easier to follow a similar approach to the port flags, and have default values in the root.go file that can be changed with flags.

This would remove the need for the customerOptions struct altogether, and the init function in each service would just have the one line:

RootCmd.AddCommand(customerCmd)

@cboornaz17
Copy link
Contributor Author

Also, I just discovered that in each service's client.go file, the service searches on the original hardcoded hostPort value, so it does not find the service if the port has been changed.
For example. services/driver/client.go line 50:

HostPort: "127.0.0.1:8082"

or in services/route/route.go line 56:

url := "http://127.0.0.1:8083/route?" + v.Encode()

I am looking into how to share the variable across packages, but maybe having them in the config file is the way to go?

I appreciate your help and patience on something so minor

@yurishkuro
Copy link
Member

riiiiight... Yes, we need all those references to be accessible from some central location or passed in a combined configOptions struct, similar to the former customerOptions but shared between all services. My preference is the options struct rather than using global variables in /config/

@cboornaz17
Copy link
Contributor Author

cboornaz17 commented Jul 24, 2018

So if taking the configOptions struct approach, it would be one struct containing the ports for each service?

Then this one struct could be created in the cmd/frontend.go file, which would create a new frontend server with the struct, which would then pass each port to the proper service when it spins up each service client?

Gonna get to work.

@cboornaz17 cboornaz17 force-pushed the add-cmd-flags-hotrod-ports branch 4 times, most recently from 2bbb3ec to 43a5d22 Compare July 25, 2018 04:06
Signed-off-by: Corey Boornazian <coreyb220@gmail.com>

Update client.go

Signed-off-by: Corey Boornazian <coreyb220@gmail.com>
Signed-off-by: Corey Boornazian <coreyb220@gmail.com>

Fix typo on root.go

Signed-off-by: Corey Boornazian <coreyb220@gmail.com>
@cboornaz17
Copy link
Contributor Author

Hello, just wondering if there was anything else I could add/change to get this PR accepted.

Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, didn't see the last update.

LGTM! Thanks for your contribution.

@yurishkuro yurishkuro merged commit 0b75852 into jaegertracing:master Aug 18, 2018
isaachier pushed a commit to isaachier/jaeger that referenced this pull request Sep 3, 2018
)

* Add ConfigOptions so clients find correct ports

Signed-off-by: Corey Boornazian <coreyb220@gmail.com>

Update client.go

Signed-off-by: Corey Boornazian <coreyb220@gmail.com>

* Fix typo on client.go

Signed-off-by: Corey Boornazian <coreyb220@gmail.com>

Fix typo on root.go

Signed-off-by: Corey Boornazian <coreyb220@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants