-
Notifications
You must be signed in to change notification settings - Fork 165
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 a flag to start specifying log index ranges for virtual indices. #435
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense so far
cmd/rekor-server/app/root.go
Outdated
@@ -60,6 +61,8 @@ func init() { | |||
rootCmd.PersistentFlags().String("trillian_log_server.address", "127.0.0.1", "Trillian log server address") | |||
rootCmd.PersistentFlags().Uint16("trillian_log_server.port", 8090, "Trillian log server port") | |||
rootCmd.PersistentFlags().Uint("trillian_log_server.tlog_id", 0, "Trillian tree id") | |||
rootCmd.PersistentFlags().Var(&logRangeMap, "trillian_log_server.log_id_ranges", "list of tree ids and ranges") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any need to specify a different log server IP/port for each tree ID here, or is a single trillian instance sufficient for multiple trees?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could see us needing to eventually, but can't think of a real reason yet. WDYT? One trillian instance can handle many trees.
cmd/rekor-server/app/flags_test.go
Outdated
{TreeId: 1, TreeLength: 17}, | ||
{TreeId: 2, TreeLength: 1}, | ||
{TreeId: 3, TreeLength: 100}, | ||
{TreeId: 4}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense here (for all new additions to go into TreeId: 4
), but should we extend the command line arg to have a trailing value to match this? If the server panics and restarts, we don't want it to create a new tree but if the trillian instance reports back more than one tree that isn't specified in the list we wouldn't be able to disambiguate which one to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, I thought about this but skipped it. We should. I'll fix it up here.
This is part of the temporal sharding work. The flag is not hooked up anywhere yet. Signed-off-by: Dan Lorenc <dlorenc@google.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if Bob's points are resolved.
This is part of the temporal sharding work. The flag is not
hooked up anywhere yet.
Signed-off-by: Dan Lorenc dlorenc@google.com