-
Notifications
You must be signed in to change notification settings - Fork 46
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
refactor: simplify job cli #944
Conversation
Codecov Report
@@ Coverage Diff @@
## master #944 +/- ##
======================================
Coverage 36.5% 36.5%
======================================
Files 27 27
Lines 1849 1849
======================================
Hits 675 675
Misses 1099 1099
Partials 75 75 |
Name: "tasks", | ||
Usage: "Comma separated list of tasks to run in job. Each task is reported separately in the storage backend.", | ||
EnvVars: []string{"LILY_JOB_TASKS"}, | ||
Value: cli.NewStringSlice(tasktype.AllTableTasks...), |
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.
by default we enable all tasks.
This means calls to the survey command will error when passed the default values, and callers will need to explicit set tasks to its expected values - peeragents
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.
That's not a huge deal. But ideally there's an override Flag which inherits the regular taskFlag fields but sets the Value appropriately. I leave this up to you.
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.
Users can still do that with this flag: lily job run --tasks=peeragents survey
overrides the default values of this flag, as does lily job run --task=block_header watch
.
var RangeFromFlag = &cli.Int64Flag{ | ||
Name: "from", | ||
Usage: "Limit actor and message processing to tipsets at or above `HEIGHT`", | ||
EnvVars: []string{"LILY_FROM"}, | ||
Destination: &rangeFlags.from, | ||
Required: true, | ||
} | ||
|
||
var RangeToFlag = &cli.Int64Flag{ | ||
Name: "to", | ||
Usage: "Limit actor and message processing to tipsets at or below `HEIGHT`", | ||
EnvVars: []string{"LILY_TO"}, | ||
Destination: &rangeFlags.to, | ||
Required: true, | ||
} |
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.
reusable flags by all commands that accept a range: walk
find
and fill
|
||
var rangeFlags rangeOps | ||
|
||
func (r rangeOps) validate() error { |
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.
called in the before function on walk
fill
and find
commands
@@ -14,10 +14,10 @@ import ( | |||
"github.com/filecoin-project/lily/lens/lily" | |||
) | |||
|
|||
func GetAPI(ctx context.Context, addrStr string, token string) (lily.LilyAPI, jsonrpc.ClientCloser, error) { | |||
addrStr = strings.TrimSpace(addrStr) | |||
func GetAPI(ctx context.Context) (lily.LilyAPI, jsonrpc.ClientCloser, error) { |
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.
looks at flags rather than accept them as arguments, decrease the opportunity for developer error.
@@ -21,12 +21,12 @@ type Filler struct { | |||
DB *storage.Database |
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.
changes here due to unify range flags (--to and --from) to use int64's. mechanical change
@@ -16,12 +16,12 @@ type Finder struct { | |||
DB *storage.Database |
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.
changes here due to unify range flags (--to and --from) to use int64's. mechanical change
@@ -19,8 +19,8 @@ import ( | |||
) |
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.
changes here due to unify range flags (--to and --from) to use int64's. mechanical change
@@ -18,12 +18,12 @@ type Notifier struct { | |||
queue *queue.AsynQ |
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.
changes here due to unify range flags (--to and --from) to use int64's. mechanical change
@@ -9,14 +9,15 @@ import ( | |||
|
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.
mechanical changes to remove client flags
@@ -4,10 +4,11 @@ import ( | |||
"fmt" |
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.
mechanical changes to remove client flags and rename Visor* flags to Lily*
@@ -29,12 +29,9 @@ var NetCmd = &cli.Command{ | |||
var NetID = &cli.Command{ |
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.
mechanical to remove client flags
@@ -29,23 +29,23 @@ import ( | |||
|
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.
mechanical to rename Visor to lily
@@ -8,12 +8,9 @@ import ( | |||
var StopCmd = &cli.Command{ |
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.
mechanical to remove client flags
@@ -6,12 +6,13 @@ import ( | |||
"time" |
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.
mechanical to remove client flags
@@ -13,16 +13,13 @@ import ( | |||
var WaitApiCmd = &cli.Command{ | |||
Name: "wait-api", |
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.
mechanical to remove client flags
Name string | ||
Tasks []string | ||
Queue string | ||
type LilyJobConfig struct { |
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.
Config used by all configs used to submit jobs
@@ -75,13 +75,13 @@ func (m *LilyNodeAPI) ChainGetTipSetAfterHeight(ctx context.Context, epoch abi.C | |||
|
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.
mechanical change to use JobConfig
struct
@@ -492,7 +492,7 @@ func GenerateUpsertStrings(model interface{}) (string, string) { | |||
} | |||
|
|||
// returns a map of heights to missing tasks, and a list of heights to iterate the map in order with. |
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.
mechanical change to use int64's
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.
A bunch of really small details. This is a great refactor. 🙌 Sticky approve to merge. 👍
commands/job/options.go
Outdated
} | ||
if len(RunFlags.Tasks.Value()) == 0 { | ||
// TODO don't panic | ||
panic("need tasks") |
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.
nit: A more descriptive panic would be nice.
type LilyIndexNotifyConfig struct { | ||
IndexConfig LilyIndexConfig | ||
|
||
Queue string | ||
} |
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.
Hooray struct composition!
commands/job/index.go
Outdated
Usage: "Index the state of a tipset from the filecoin blockchain by height", | ||
Usage: "Index the state of a tipset from the filecoin blockchain by height.", | ||
Description: ` | ||
Index the state of a tipset from the filecoin blockchain by height. If the provided height is a null-round and error will be returned. |
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.
If the provided height is a null-round, an
error will be returned.
commands/job/watch.go
Outdated
to the --workers flag will allow the watch job to index tipsets simultaneously (Note: this will use a significant amount of system resources). | ||
|
||
Since it may be the case that lily experiences a reorg while the watch job is observing the head of the chain | ||
the --confidence flag may be used to buffed the amount of tipsets observed before it begins indexing - illustrated by the below diagram: |
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.
to buffer
while 0 means none should be traced. No intermediate | ||
values are accepted. For a 'probabilistic' sampler | ||
the param indicates the fraction of function calls | ||
that should be sampled. |
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 think a lot of these hidden flags and envvars are not documented anywhere. If this is true, could we see about including them somewhere appropriate?
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.
They are via:
./lily --help
<omitted some details>
GLOBAL OPTIONS:
--api value Address of lily api in multiaddr format. (default: "/ip4/127.0.0.1/tcp/1231") [$LILY_API]
--api-token value Authentication token for lily api. [$LILY_API_TOKEN]
--log-level LEVEL Set the default log level for all loggers to LEVEL (default: "info") [$GOLOG_LOG_LEVEL]
--log-level-named value A comma delimited list of named loggers and log levels formatted as name:level, for example 'logger1:debug,logger2:info' [$LILY_LOG_LEVEL_NAMED]
--jaeger-tracing (default: false) [$LILY_JAEGER_TRACING]
--jaeger-service-name value (default: "lily") [$LILY_JAEGER_SERVICE_NAME]
--jaeger-provider-url value (default: "http://localhost:14268/api/traces") [$LILY_JAEGER_PROVIDER_URL]
--jaeger-sampler-ratio value If less than 1 probabilistic metrics will be used. (default: 1) [$LILY_JAEGER_SAMPLER_RATIO]
--prometheus-port value (default: ":9911") [$LILY_PROMETHEUS_PORT]
--redis-addr value Redis server address in "host:port" format (default: "127.0.0.1:6379") [$LILY_REDIS_ADDR]
--redis-username value Username to authenticate the current connection when redis ACLs are used. [$LILY_REDIS_USERNAME]
--redis-password value Password to authenticate the current connection [$LILY_REDIS_PASSWORD]
--redis-db value Redis DB to select after connection to server (default: 0) [$LILY_REDIS_DB]
--version, -v print the version (default: false)
Name: "tasks", | ||
Usage: "Comma separated list of tasks to run in job. Each task is reported separately in the storage backend.", | ||
EnvVars: []string{"LILY_JOB_TASKS"}, | ||
Value: cli.NewStringSlice(tasktype.AllTableTasks...), |
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.
That's not a huge deal. But ideally there's an override Flag which inherits the regular taskFlag fields but sets the Value appropriately. I leave this up to you.
Whats changed?
watch
,walk
,index
,find
,fill
, andsurvey
jobs have changesVisor*
structs toLily*
All jobs are now grouped under the job command
jobs are runnable via the
job run
command:Since all jobs accept
window
,tasks
,storage
,name
,restart-delay
,restart-on-failure
, andrestart-on-completion
options those flags now exist on thejob run
commandwalk
job descriptionwatch
job descriptionfind
job descriptionfill
job descriptionJobs capable of notifying for distributed indexing now have a notify command
watch notify
walk notify
fill notify
--api
and--api-token
are now flags on thelily
command only (previously each job took these flags, which was annoying)TODO: