tools: Fix tm bench quit #2405

bradyjoestar · 2018-09-15T07:34:57Z

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG_PENDING.md

Develop

ValarDragon

lgtm

melekes

Thanks for contributing to Tendermint. A few changes must be made before merging. NOTE you can setup gofmt to automatically format your code in almost any popular editor https://robots.thoughtbot.com/writing-go-in-vim https://atom.io/packages/go-plus https://github.com/microsoft/vscode-go

melekes · 2018-09-17T07:10:09Z

tools/tm-bench/main.go

@@ -173,3 +177,13 @@ func startTransacters(

 	return transacters
 }
+
+// RunForever waits for an interrupt signal and stops the node.
+func  catchInterrupt(transacters []*transacter) {


formatting is off (please use https://golang.org/cmd/gofmt/)

I'm surprised this wasn't caught by linting. We should probably consider adding a second step then, like we do in the SDK, to make sure gofmt is correct.

melekes · 2018-09-17T07:10:47Z

tools/tm-bench/main.go

+
+// RunForever waits for an interrupt signal and stops the node.
+func  catchInterrupt(transacters []*transacter) {
+	// Sleep forever and then...


please remove (comment is useless)

melekes · 2018-09-17T07:11:03Z

tools/tm-bench/main.go

+func  catchInterrupt(transacters []*transacter) {
+	// Sleep forever and then...
+	cmn.TrapSignal(func() {
+		for _,e := range transacters {


melekes · 2018-09-17T07:12:47Z

tools/tm-bench/main.go

@@ -93,6 +94,9 @@ Examples:
 		"broadcast_tx_"+broadcastTxMethod,
 	)

+	//catch Interrupt and quit tm-bench
+	go catchInterrupt(transacters)


I'd remove catchInterrupt altogether and just call

go cmn.TrapSignal(func() { ... }

why need in intermediate function?

Thanks for your review! All was done according to your nice advice.

xla · 2018-09-17T14:50:50Z

tools/tm-bench/main.go

@@ -93,6 +94,13 @@ Examples:
 		"broadcast_tx_"+broadcastTxMethod,
 	)

+	//catch Interrupt and quit tm-bench
+	go cmn.TrapSignal(func() {


It doesn't seem advisable to further use this method, it logs uncontrolled and the functionality is simple enough that it could be copied as is.

I do agree, but I think we can remove all the cmn.TrapSignals at once in a separate PR.

Its already written, who knows when we're going to get to deleting / refactoring the trap signal logic.

codecov-io · 2018-09-19T11:16:58Z

Codecov Report

Merging #2405 into develop will decrease coverage by 0.02%.
The diff coverage is 0%.

@@             Coverage Diff             @@
##           develop    #2405      +/-   ##
===========================================
- Coverage    60.99%   60.96%   -0.03%     
===========================================
  Files          197      197              
  Lines        16313    16320       +7     
===========================================
  Hits          9950     9950              
- Misses        5496     5505       +9     
+ Partials       867      865       -2

Impacted Files	Coverage Δ
tools/tm-bench/main.go	`0% <0%> (ø)`	⬆️
blockchain/reactor.go	`39.1% <0%> (-0.34%)`	⬇️
consensus/reactor.go	`72.75% <0%> (-0.27%)`	⬇️
p2p/pex/pex_reactor.go	`74.33% <0%> (+0.33%)`	⬆️
blockchain/pool.go	`66.43% <0%> (+0.69%)`	⬆️

melekes · 2018-09-20T07:31:47Z

tools/tm-bench/main.go

@@ -93,6 +95,19 @@ Examples:
 		"broadcast_tx_"+broadcastTxMethod,
 	)

+	//catch Interrupt and quit tm-bench


I don't think this comment is needed

Should be removed or have proper form of: Catch interrupt and quit.

xla

Some style and functional changes needed.

xla · 2018-09-20T08:48:44Z

tools/tm-bench/main.go

@@ -12,6 +12,8 @@ import (

 	"github.com/tendermint/tendermint/libs/log"
 	tmrpc "github.com/tendermint/tendermint/rpc/client"
+	"os/signal"
+	"syscall"


Import order is as follows:

import ( // stdlib // external packages // internal packages )

I wonder if we can capture this in the linter ?!

I wrote this so many times by now, was wondering the same.

xla · 2018-09-20T08:49:11Z

tools/tm-bench/main.go

@@ -93,6 +95,19 @@ Examples:
 		"broadcast_tx_"+broadcastTxMethod,
 	)

+	//catch Interrupt and quit tm-bench


Should be removed or have proper form of: Catch interrupt and quit.

xla · 2018-09-20T08:54:20Z

tools/tm-bench/main.go

@@ -93,6 +95,19 @@ Examples:
 		"broadcast_tx_"+broadcastTxMethod,
 	)

+	//catch Interrupt and quit tm-bench
+	go func() {


This should be moved up, before the first side-effectful call on line 86. It also doesn't need to be wrapped in a goroutine entirely, only the part which ranges over the channel.

xla · 2018-09-20T08:54:49Z

tools/tm-bench/main.go

+			for _, t := range transacters {
+				t.Stop()
+			}
+			os.Exit(1)


Is it an error if the user quits on purpose? Would challenge that and rethink if the binary exit with an error code.

xla · 2018-09-25T10:46:11Z

@bradyjoestar Gentle ping on this. Would you be able to address the latest comments? Then it should be good to go.

I'll address comments in a separate PR

#2405 (review)

* specify time unit for FlushThrottleTimeout in TestP2PConfig Refs #2555 * [tm-bench] refactor code #2405 (review)

bradyjoestar and others added 3 commits September 14, 2018 15:02

Merge pull request #4 from tendermint/develop

b556aaa

Develop

fix tm-bench catchInterupt

ae17b9d

add CHANGELOG

ce0cd94

bradyjoestar requested review from ebuchman, melekes, xla and zramsay as code owners September 15, 2018 07:34

bradyjoestar changed the base branch from master to develop September 15, 2018 07:36

ValarDragon approved these changes Sep 15, 2018

View reviewed changes

fix codecov/patch

27d94a4

melekes suggested changes Sep 17, 2018

View reviewed changes

gofmt & remove intermediate function

e087f9b

melekes approved these changes Sep 17, 2018

View reviewed changes

xla suggested changes Sep 17, 2018

View reviewed changes

fix log uncontrolled

c04b91b

melekes approved these changes Sep 20, 2018

View reviewed changes

melekes reviewed Sep 20, 2018

View reviewed changes

xla previously requested changes Sep 20, 2018

View reviewed changes

xla removed request for ebuchman and zramsay September 20, 2018 08:55

xla self-assigned this Sep 25, 2018

xla added T:enhancement Type: Enhancement tools labels Sep 25, 2018

xla changed the title ~~Fix tm bench quit~~ tools: Fix tm bench quit Sep 25, 2018

melekes merged commit 4c0c6e0 into tendermint:develop Oct 8, 2018

melekes added a commit that referenced this pull request Oct 8, 2018

[tm-bench] refactor code

a2eae12

#2405 (review)

xla pushed a commit that referenced this pull request Oct 8, 2018

tools: Refactor tm-bench (#2570)

35b6712

* specify time unit for FlushThrottleTimeout in TestP2PConfig Refs #2555 * [tm-bench] refactor code #2405 (review)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools: Fix tm bench quit #2405

tools: Fix tm bench quit #2405

bradyjoestar commented Sep 15, 2018 •

edited by xla

ValarDragon left a comment

melekes left a comment

melekes Sep 17, 2018

ValarDragon Sep 17, 2018

melekes Sep 17, 2018

melekes Sep 17, 2018

melekes Sep 17, 2018

bradyjoestar Sep 17, 2018

xla Sep 17, 2018

ValarDragon Sep 20, 2018

codecov-io commented Sep 19, 2018

melekes Sep 20, 2018

xla Sep 20, 2018

xla left a comment

xla Sep 20, 2018

ebuchman Sep 20, 2018

xla Sep 21, 2018

xla Sep 20, 2018

xla Sep 20, 2018

xla Sep 20, 2018

xla commented Sep 25, 2018

tools: Fix tm bench quit #2405

tools: Fix tm bench quit #2405

Conversation

bradyjoestar commented Sep 15, 2018 • edited by xla

ValarDragon left a comment

Choose a reason for hiding this comment

melekes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Sep 19, 2018

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xla commented Sep 25, 2018

bradyjoestar commented Sep 15, 2018 •

edited by xla