Add flags to allow changing ports for HotROD services #951

cboornaz17 · 2018-07-23T21:03:19Z

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 · 2018-07-24T00:31:22Z

Codecov Report

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

@@          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.

yurishkuro · 2018-07-24T01:17:35Z

examples/hotrod/cmd/root.go

+	fixCustomerPort int
+	fixDriverPort int
+	fixFrontendPort int
+	fixRoutePort int


"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".

yurishkuro · 2018-07-24T01:18:43Z

examples/hotrod/cmd/customer.go

@@ -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)),


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

yurishkuro · 2018-07-24T01:19:35Z

examples/hotrod/cmd/root.go

+	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")


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

yurishkuro · 2018-07-24T01:20:56Z

examples/hotrod/cmd/root.go

@@ -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))


No need for "fix: " prefix

yurishkuro · 2018-07-24T01:23:08Z

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 · 2018-07-24T01:23:55Z

Thanks for the PR

cboornaz17 · 2018-07-24T01:48:29Z

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 · 2018-07-24T03:38:48Z

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 · 2018-07-24T03:49:07Z

examples/hotrod/services/config/config.go

+	// Port settings, can be overwritten from command line
+
+	// Customer Service Port
+	CustomerPort = 8081


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 · 2018-07-24T03:49:29Z

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 · 2018-07-24T03:56:49Z

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 · 2018-07-24T04:00:50Z

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 · 2018-07-24T19:33:59Z

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 · 2018-07-24T19:53:00Z

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 · 2018-07-24T20:00:01Z

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 · 2018-07-24T20:11:45Z

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.

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 · 2018-08-18T17:24:53Z

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

yurishkuro

Sorry, didn't see the last update.

LGTM! Thanks for your contribution.

) * 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>

cboornaz17 requested review from black-adder, jpkrohling, pavolloffay, vprithvi and yurishkuro as code owners July 23, 2018 21:03

yurishkuro reviewed Jul 24, 2018

View reviewed changes

cboornaz17 force-pushed the add-cmd-flags-hotrod-ports branch from 749095b to ddbc1a7 Compare July 24, 2018 01:46

cboornaz17 force-pushed the add-cmd-flags-hotrod-ports branch from ddbc1a7 to 8cef0fe Compare July 24, 2018 03:12

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

yurishkuro reviewed Jul 24, 2018

View reviewed changes

cboornaz17 force-pushed the add-cmd-flags-hotrod-ports branch from 85d487f to b7d0540 Compare July 24, 2018 19:28

cboornaz17 force-pushed the add-cmd-flags-hotrod-ports branch 4 times, most recently from 2bbb3ec to 43a5d22 Compare July 25, 2018 04:06

Add ConfigOptions so clients find correct ports

327f1d1

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

cboornaz17 force-pushed the add-cmd-flags-hotrod-ports branch from c8a12fa to 327f1d1 Compare July 25, 2018 04:15

Fix typo on client.go

589331d

Signed-off-by: Corey Boornazian <coreyb220@gmail.com> Fix typo on root.go Signed-off-by: Corey Boornazian <coreyb220@gmail.com>

cboornaz17 force-pushed the add-cmd-flags-hotrod-ports branch from 74b987f to 589331d Compare July 25, 2018 15:22

cboornaz17 added 2 commits August 2, 2018 17:39

Merge branch 'master' into add-cmd-flags-hotrod-ports

34ffdde

Merge branch 'master' into add-cmd-flags-hotrod-ports

424b30f

yurishkuro approved these changes Aug 18, 2018

View reviewed changes

yurishkuro merged commit 0b75852 into jaegertracing:master Aug 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flags to allow changing ports for HotROD services #951

Add flags to allow changing ports for HotROD services #951

cboornaz17 commented Jul 23, 2018

codecov bot commented Jul 24, 2018 •

edited

Loading

yurishkuro Jul 24, 2018

yurishkuro Jul 24, 2018

yurishkuro Jul 24, 2018

yurishkuro Jul 24, 2018

yurishkuro commented Jul 24, 2018

yurishkuro commented Jul 24, 2018

cboornaz17 commented Jul 24, 2018

yurishkuro commented Jul 24, 2018 •

edited

Loading

yurishkuro Jul 24, 2018

yurishkuro commented Jul 24, 2018

cboornaz17 commented Jul 24, 2018

yurishkuro commented Jul 24, 2018

cboornaz17 commented Jul 24, 2018

cboornaz17 commented Jul 24, 2018

yurishkuro commented Jul 24, 2018

cboornaz17 commented Jul 24, 2018 •

edited

Loading

cboornaz17 commented Aug 18, 2018

yurishkuro left a comment

Add flags to allow changing ports for HotROD services #951

Add flags to allow changing ports for HotROD services #951

Conversation

cboornaz17 commented Jul 23, 2018

Which problem is this PR solving?

Short description of the changes

codecov bot commented Jul 24, 2018 • edited Loading

Codecov Report

yurishkuro Jul 24, 2018

Choose a reason for hiding this comment

yurishkuro Jul 24, 2018

Choose a reason for hiding this comment

yurishkuro Jul 24, 2018

Choose a reason for hiding this comment

yurishkuro Jul 24, 2018

Choose a reason for hiding this comment

yurishkuro commented Jul 24, 2018

yurishkuro commented Jul 24, 2018

cboornaz17 commented Jul 24, 2018

yurishkuro commented Jul 24, 2018 • edited Loading

yurishkuro Jul 24, 2018

Choose a reason for hiding this comment

yurishkuro commented Jul 24, 2018

cboornaz17 commented Jul 24, 2018

yurishkuro commented Jul 24, 2018

cboornaz17 commented Jul 24, 2018

cboornaz17 commented Jul 24, 2018

yurishkuro commented Jul 24, 2018

cboornaz17 commented Jul 24, 2018 • edited Loading

cboornaz17 commented Aug 18, 2018

yurishkuro left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 24, 2018 •

edited

Loading

yurishkuro commented Jul 24, 2018 •

edited

Loading

cboornaz17 commented Jul 24, 2018 •

edited

Loading