Skip to content
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

Remove deprecated code #5894

Merged
merged 9 commits into from
Jul 2, 2019
Merged

Remove deprecated code #5894

merged 9 commits into from
Jul 2, 2019

Conversation

preetapan
Copy link
Member

@preetapan preetapan commented Jun 27, 2019

Removed upgrade path code and tests related to:

  • Namespaces (Available since 0.7.0)
  • SubmitTime/Version (0.6.0)
  • Job Summaries (0.4.0)

Copy link
Contributor

@notnoop notnoop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall look good to me but some questions to understand implication:

  • Should "Remove in 0.9" sections be removed right before 0.10 or anytime after 0.9.0? Given that the drain request format changed in 0.8, it'd feel a bit odd dropping support for old format in 0.9.4.
  • For the FSM 0.7 compatibility changes - does dropping these means long running nomad servers must at least snapshot once between upgrading from 0.7 to 0.9.4?

@@ -169,7 +169,7 @@ func (c *JobStatusCommand) Run(args []string) int {
basic := []string{
fmt.Sprintf("ID|%s", *job.ID),
fmt.Sprintf("Name|%s", *job.Name),
fmt.Sprintf("Submit Date|%s", formatTime(getSubmitTime(job))),
fmt.Sprintf("Submit Date|%s", formatTime(time.Unix(0, *job.SubmitTime))),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the utility function here, as it's not obvious that job.Submit is in nano seconds.

@preetapan
Copy link
Member Author

preetapan commented Jun 27, 2019

overall look good to me but some questions to understand implication:

  • Should "Remove in 0.9" sections be removed right before 0.10 or anytime after 0.9.0? Given that the drain request format changed in 0.8, it'd feel a bit odd dropping support for old format in 0.9.4.

That's fair, perhaps one major version is too short of a deprecation cycle for node drain. I could roll back these changes. In 0.9.4 we could start emitting a warning when servicing requests that use the old format. .

  • For the FSM 0.7 compatibility changes - does dropping these means long running nomad servers must at least snapshot once between upgrading from 0.7 to 0.9.4?

Nomad 0.7 added namespaces and introduced code to fix up older state store objects that were missing namespaces. As long as the servers were making raft transactions it should have corrected old objects that don't have namespace set (not necessarily tied to snapshots)

@notnoop
Copy link
Contributor

notnoop commented Jun 28, 2019

Nomad 0.7 added namespaces and introduced code to fix up older state store objects that were missing namespaces

Cool, let me clarify: Would nomad 0.7 edit the raft log old persisted entries in place in file and how do we do that? I was expecting Nomad 0.7 to parse old log entries and populate missing namespace fields in memory, and then storing updated entries to raft snapshot without updating old entries.

@preetapan
Copy link
Member Author

preetapan commented Jul 1, 2019

Would nomad 0.7 edit the raft log old persisted entries in place in file and how do we do that? I was expecting Nomad 0.7 to parse old log entries and populate missing namespace fields in memory, and then storing updated entries to raft snapshot without updating old entries.

AFAICT we use the canonicalize method in restore to fix up jobs, but we dont do that consistently for all objects. You are right about your understanding though - The code I removed in various upsert methods would handle processing old log entries - when upserting those entries into the state store Nomad 0.7 would add the namespace field. At that point the statestore in memory would have namespace in it, but not the old log entries on disk. Once we are past the snapshot threshold (8192 entries) we would take a snapshot which saves the statestore (now with namespace in it) to disk, and delete old log entries.

@preetapan preetapan merged commit 0819e4a into master Jul 2, 2019
@preetapan preetapan deleted the f-remove-deprecated-code branch October 17, 2019 14:10
@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants