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 error from mismatched plan state #15460

Merged
merged 1 commit into from
Jul 3, 2017

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Jul 3, 2017

Plan application was using the wrong copy of the state, causing serial
numbers to step backewards. After fixing this a sanity check comparing
the states was added, but that fails due to some states be altered by
the serialization itself.

Change the error condition to only log the error for reference, as these
state should be equal. We can address the overall state handling in the
next major release, hopefully negating the need for any of these
changes.

Plan application was using the wrong copy of the state, causing serial
numbers to step backewards. After fixing this a sanity check comparing
the states was added, but that fails due to some states be altered by
the serialization itself.

Change the error condition to only log the error for reference, as these
state should be equal. We can address the overall state handling in the
next major release, hopefully negating the need for any of these
changes.
// the state, there is little chance that these aren't actually equal.
// Log the error condition for reference, but continue with the state
// we have.
log.Println("[ERROR] plan state and ContextOpts state are not equal")
Copy link
Contributor

Choose a reason for hiding this comment

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

Given how much of a pain it was to get to the bottom of the error here, would it be worth us including some string representation of the state in the log in this case? I guess it might be a bit noisy to dump two whole states, but since it only happens in this exceptional path maybe it's okay? 🤷‍♂️

Copy link
Member Author

@jbardin jbardin Jul 3, 2017

Choose a reason for hiding this comment

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

Since the serialization process is what's is causing the change, only the raw in-memory representation is going to show any difference. I did try this using spew, but the output was very noisy, and we would basically need to create a post-processing tool to diff the pages of output. I think since the case that's causing this more common than we thought, we should refrain from dumping the entire state into the logs, twice.

Once we take care of the serialization issues we can make this complain louder.

@jbardin jbardin merged commit 6ca4585 into v0.9-maint Jul 3, 2017
@jbardin jbardin deleted the jbardin/apply-plan-state-equal branch July 3, 2017 18:06
@ghost
Copy link

ghost commented Apr 8, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants