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

Revert "Reduce fileclient.get_file latency by merging _file_find and … #43421

Merged
merged 1 commit into from Sep 11, 2017

Conversation

gtmanfred
Copy link
Contributor

…_file_hash"

This reverts commit 94c6238.

Revert this commit because it breaks backwards compatibility with older masters.

@ninja- if you wanted to submit a new change to reimplement this, we would greatly appreciate it, but it is causing breakage when running older masters and 2017.7 minions.

@thatch45

Thanks,
Daniel

@ghost
Copy link

ghost commented Sep 8, 2017

@gtmanfred, thanks for your PR! By analyzing the history of the files in this pull request, we identified @ninja-, @thatch45 and @terminalmage to be potential reviewers.

@rallytime rallytime added the bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch label Sep 8, 2017
@ninja-
Copy link
Contributor

ninja- commented Sep 9, 2017

@gtmanfred I thought we agreed users should upgrade masters before they upgrade minions ???

@ninja-
Copy link
Contributor

ninja- commented Sep 9, 2017

@gtmanfred um how about

try {
ret = _file_hash_and_stat(...)
} catch (e) {
ret[0] = _file_find(...)
ret[1] = _file_hash(...)
}

?

It will add a bit of latency to these with old master, but will keep the 'access time cut in half' benefits for users which upgrade the master.
Or maybe a smarter if condition that checks protocol version, but I don't think there's such a thing as protocol version here.

@gtmanfred
Copy link
Contributor Author

@ninja- yeah, technically that is correct, but we really prefer to not break that backwards compatibility.

After talking to @thatch45 we decided that this didn't give enough benefit as is to justify breaking backwards compatibilty. I am on vacation starting a couple hours ago, but I think if you can check that that works for both older minions and newer minions, then I would bet that we can get this in, but please open a PR as soon as possible because we are looking at freezing and testing the 2017.7.2 release soon.

@rallytime @Ch3LL

Thanks,
Daniel

@thatch45
Copy link
Member

thatch45 commented Sep 9, 2017

I think that I need to add a clarification here, while we advise users to upgrade the master first as best practice it should be required only as a last resort. If it is ever possible to maintain compatibility with old masters it should be done.
The project and users typically sustain a fair amount of damage from master/minion compatibility issues and we have avoided them for a long time now. The goal is that any supported releases of Salt can have bi-directional communication without issue.

@cachedout cachedout merged commit 673ce38 into saltstack:2017.7 Sep 11, 2017
@rallytime rallytime added ZZZ[Done]-back-ported-bf RETIRED The pull request has been back-ported to an older branch. and removed bugfix-bckport will be be back-ported to an older release branch by creating a PR against that branch labels Sep 11, 2017
cachedout pushed a commit that referenced this pull request Sep 11, 2017
@gtmanfred gtmanfred deleted the compat branch January 11, 2018 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZRELEASED - 2017.7.2 ZZZ[Done]-back-ported-bf RETIRED The pull request has been back-ported to an older branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants