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

poolsize environment attribute breaks upgrades #506

Closed
soamvasani opened this Issue Feb 22, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@soamvasani
Member

soamvasani commented Feb 22, 2018

The addition of the poolsize attribute to environment spec broke upgrades. Here's why:

  1. New environment spec has a poolsize attribute
  2. But old environments don't have poolsize defined
  3. Reading old environments into the new env spec yields poolsize equal to zero
  4. Poolsize==0 causes functions using poolmgr to fail

There's a lot of options on how to fix this:

  • Option 1: Use env versioning -- create a new environment version (3) and only read poolsize when version>=3.
  • Option 2: Ignore poolsize==0 when functions exist that use poolmgr
  • Option 3: Dispatch functions to newdeploy executor backend when poolsize==0.

Option 1 seems cleanest to think about.

@vishal-biyani

This comment has been minimized.

Collaborator

vishal-biyani commented Feb 26, 2018

Closed by PR.

@life1347

This comment has been minimized.

Member

life1347 commented Sep 11, 2018

Do we consider using pointer(*int) for Poolsize? Another option is use string to distinguish 0 and nil here. That means if a env is in old schema than it will be "" instead of zero and for new schema it will be "0".
Be honest, it not so useful to add new env version with only one config. It makes users confused what are the differences between 1, 2 and 3 env version.

@soamvasani

This comment has been minimized.

Member

soamvasani commented Sep 11, 2018

Wouldn't changing int to int* also be a compatibility-breaking change?

@life1347

This comment has been minimized.

Member

life1347 commented Sep 12, 2018

yes, so we may need to add another field for this purpose.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment