[Consensus] Implement startup-time flag #1304
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1304 +/- ##
==========================================
+ Coverage 55.01% 56.22% +1.21%
==========================================
Files 501 500 -1
Lines 31739 31073 -666
==========================================
+ Hits 17460 17471 +11
+ Misses 11922 11237 -685
- Partials 2357 2365 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
cmd/collection/main.go
Outdated
| @@ -139,13 +141,25 @@ func main() { | |||
| "additional fraction of replica timeout that the primary will wait for votes") | |||
| flags.DurationVar(&blockRateDelay, "block-rate-delay", 250*time.Millisecond, | |||
| "the delay to broadcast block proposal in order to control block production rate") | |||
| flags.StringVar(&startupTimeString, "startup-time", cmd.NotSet, | |||
| "specifies date and time (in ISO 8601 format) when the consensus participant enters the first view (e.g 2006-01-02T15:04:05Z07:00)") | |||
There was a problem hiding this comment.
I don't think we need this flag for collection cluster.
Do we? @AlexHentschel
There was a problem hiding this comment.
The original issue says we need it for both.
cmd/consensus/main.go
Outdated
| @@ -143,8 +145,19 @@ func main() { | |||
| flags.StringVar(&accessAddress, "access-address", "", "the address of an access node") | |||
| flags.StringVar(&secureAccessNodeID, "secure-access-node-id", "", "the node ID of the secure access GRPC server") | |||
| flags.BoolVar(&insecureAccessAPI, "insecure-access-api", true, "required if insecure GRPC connection should be used") | |||
| flags.StringVar(&startupTimeString, "startup-time", cmd.NotSet, "specifies date and time (in ISO 8601 format) when the consensus participant enters the first view") | |||
There was a problem hiding this comment.
Please also add the time format in the comment here as well.
Also it's better to mention that if this startup time is a time in the past, then the node will start up right away.
There was a problem hiding this comment.
Updated doc to reflect that start-time is the data after which the first view may be entered. Therefore if the start-time is in the past, then the first view may be entered immediately
@zhangchiqing @durkmurder so personally, I think the ISO format makes more sense because it is human readable, and it is also the most commonly used time format on the internet. The Z0:700 is just the time Zone. In the end it doesn't matter too much, but I'd like to push back at least a little bit on the idea that timestamps are better. They are easier and make more sense to our engineering brains, but they are more confusing and error prone for users. For example, if node operator needs to go back and check what the start time was, they can look at the command line options the node was started with. If we use timestamps, they cannot tell right away, they need to then take the timestamp and put into some tool to get human readable format. A user would definitely need to use some tool to generate the timestamp, whereas they could easily write the ISO formatted date by hand. |
There was a problem hiding this comment.
Looks good to me. Better if @AlexHentschel also give a look
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
|
bors merge |
|
looks good. |
closes https://github.com/dapperlabs/flow-go/issues/5846