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

We should only call returner.save_load once per jid #22171

Closed
basepi opened this issue Mar 30, 2015 · 7 comments
Closed

We should only call returner.save_load once per jid #22171

basepi opened this issue Mar 30, 2015 · 7 comments
Labels
Bug broken, incorrect, or confusing behavior Core relates to code central or existential to Salt P2 Priority 2 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around stale
Milestone

Comments

@basepi
Copy link
Contributor

basepi commented Mar 30, 2015

Right now we save the load in two places: once during the publish (inside of the publish function), and once inside of the _return function. This means we try to save the load again for each minion return. This is far from ideal from a performance standpoint, and also causes problems with some returners (such as mysql).

The reason we save_load on returns is in case of salt-call-initiated jobs on the minion, or jobs initiated from a lower-level master in a syndic setup.

For the former, we should be able to see from the return that it was salt-call-initiated (no jid) and save the load only then. For the latter, we're going to have to get more creative -- perhaps another syndic-specific event for saving the load?

@basepi basepi added Bug broken, incorrect, or confusing behavior severity-low 4th level, cosemtic problems, work around exists labels Mar 30, 2015
@basepi basepi added this to the Beryllium milestone Mar 30, 2015
basepi added a commit to basepi/salt that referenced this issue Mar 31, 2015
@basepi basepi added severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around Core relates to code central or existential to Salt P2 Priority 2 and removed severity-low 4th level, cosemtic problems, work around exists labels Apr 16, 2015
basepi added a commit to basepi/salt that referenced this issue Apr 27, 2015
@meggiebot meggiebot modified the milestone: Beryllium Jun 18, 2015
@jfindlay jfindlay modified the milestone: Approved Jun 19, 2015
@cachedout
Copy link
Contributor

@basepi Is this fixed now, or no?

@basepi
Copy link
Contributor Author

basepi commented Jan 4, 2016

No. We put a bandaid on it by just catching the exception on subsequent calls, if I remember correctly. But the source issue has not been fixed.

@stale
Copy link

stale bot commented Mar 4, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.

@stale stale bot added the stale label Mar 4, 2018
@stale stale bot closed this as completed Mar 11, 2018
@mattLLVW
Copy link
Contributor

you should reopen this issue. why not update load on duplicate key?

@mattp-
Copy link
Contributor

mattp- commented Jun 27, 2019

@mattLLVW this is actually fixed coincidentally
8713c1b

@mattLLVW
Copy link
Contributor

... for Postgres. The same fix should be applied to MySQL returner

@M4ttsson
Copy link

Hi, this is still an issue for some returners. I've tested both sqlite and odbc returner and both tries to save the same jid twice and fails since it is primary key as per documentation. Found this issue when looking at why the mysql returner appears to work. Sorry for waking it up again.

The second call seems to contain data that is also saved in the salt_returns table, so maybe an empty catch is a good "bandaid" since as far as I understand won't lose any data and is a very small impact compared to changing how returns work completely. So should I then try to create an issue for sqlite and odbc returners and try to fix it?

Another workaround I found is to simply create an identity column as primary key instead and allow duplicate jids. But if the return is already saved in salt_return table maybe this will only grow the database with duplicate info?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Core relates to code central or existential to Salt P2 Priority 2 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around stale
Projects
None yet
Development

No branches or pull requests

8 participants