-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Events/msgtype cleanup #9117
Events/msgtype cleanup #9117
Conversation
adds message type to signature for upsert node, update tests, remove placeholder method
update test usage of delete node delete placeholder msgtype method
631bdbc
to
e7d873f
Compare
@@ -276,7 +276,7 @@ func (n *nomadFSM) Apply(log *raft.Log) interface{} { | |||
case structs.SchedulerConfigRequestType: | |||
return n.applySchedulerConfigUpdate(buf[1:], log.Index) | |||
case structs.NodeBatchDeregisterRequestType: | |||
return n.applyDeregisterNodeBatch(buf[1:], log.Index) | |||
return n.applyDeregisterNodeBatch(msgType, buf[1:], log.Index) |
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.
commenting to highlight non test file changes
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 the applyDeregisterNodeBatch
method is always going to have this same msgType
. Does passing it as a parameter get us anything here?
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.
Thanks for highlighting the file :).
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.
@tgross mostly just for consistency and explicitness. There are only a few helper methods in fsm that use different msg types but at the time of writing them all I went with passing ¯_(ツ)_/¯
@@ -1704,13 +1704,13 @@ func (j *Job) Plan(args *structs.JobPlanRequest, reply *structs.JobPlanResponse) | |||
if oldJob.SpecChanged(args.Job) { | |||
// Insert the updated Job into the snapshot | |||
updatedIndex = oldJob.JobModifyIndex + 1 | |||
if err := snap.UpsertJob(updatedIndex, args.Job); err != nil { | |||
if err := snap.UpsertJob(structs.IgnoreUnknownTypeFlag, updatedIndex, args.Job); err != nil { |
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.
commenting to highlight non test file change, a snapshot has a no-op event publisher so this value isn't used IgnoreUnknownTypeFlag is equal to MsgTypeTestSetup
@@ -782,26 +782,11 @@ func (s *StateStore) ScalingEventsByJob(ws memdb.WatchSet, namespace, jobID stri | |||
return nil, 0, nil |
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.
commenting to highlight non test file change
…st setup msg type handle snapshot upsert eval outside of FSM and ignore eval event remove placeholder upsertevalsmsgtype handle job plan rpc and prevent event creation for plan msgtype cleanup upsertnodeevents updatenodedrain msgtype msg type 0 is a node registration event, so set the default to the ignore type
e7d873f
to
2f2696f
Compare
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.
Take a look at the structs2
thing, but my question about the parameter passing is totally a nitpick, so feel free to ignore. 😁
@@ -276,7 +276,7 @@ func (n *nomadFSM) Apply(log *raft.Log) interface{} { | |||
case structs.SchedulerConfigRequestType: | |||
return n.applySchedulerConfigUpdate(buf[1:], log.Index) | |||
case structs.NodeBatchDeregisterRequestType: | |||
return n.applyDeregisterNodeBatch(buf[1:], log.Index) | |||
return n.applyDeregisterNodeBatch(msgType, buf[1:], log.Index) |
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 the applyDeregisterNodeBatch
method is always going to have this same msgType
. Does passing it as a parameter get us anything here?
@@ -276,7 +276,7 @@ func (n *nomadFSM) Apply(log *raft.Log) interface{} { | |||
case structs.SchedulerConfigRequestType: | |||
return n.applySchedulerConfigUpdate(buf[1:], log.Index) | |||
case structs.NodeBatchDeregisterRequestType: | |||
return n.applyDeregisterNodeBatch(buf[1:], log.Index) | |||
return n.applyDeregisterNodeBatch(msgType, buf[1:], log.Index) |
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.
Thanks for highlighting the file :).
* use msgtype in upsert node adds message type to signature for upsert node, update tests, remove placeholder method * UpsertAllocs msg type test setup * use upsertallocs with msg type in signature update test usage of delete node delete placeholder msgtype method * add msgtype to upsert evals signature, update test call sites with test setup msg type handle snapshot upsert eval outside of FSM and ignore eval event remove placeholder upsertevalsmsgtype handle job plan rpc and prevent event creation for plan msgtype cleanup upsertnodeevents updatenodedrain msgtype msg type 0 is a node registration event, so set the default to the ignore type * fix named import * fix signature ordering on upsertnode to match
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
#9013 introduced a few new state store methods that were copies of the originals* with a
MsgType
explicitly added. This was mainly due to the sheer volume of test case usage. This PR removes the temporary methods and updates all the test usages to usestructs.MsgTypeTestSetup
Note to reviewers:
nomad/state/state_store.go
removes placholder methodsnomad/job_endpoint
uses structs.IgnoreUnknownMsgType as a placeholder for job planfsm.go
applyDeregisterNodeBatch
includes a message type to satisfy the used state store methods, delete events are ignored for now_test.go
files were programmatically updated, I personally wouldn't spend much time reviewing every diff in test filePlaceholder state store methods that have been removed:
UpsertNodeMsgType, DeleteNodeMsgType, UpdateNodeDrainMsgType, UpsertNodeEventsMsgType, UpsertJobMsgType, UpsertEvalsMsgType, UpsertAllocsMsgType