> On Feb. 3, 2015, 12:28 a.m., Bill Farner wrote: > > src/main/python/apache/aurora/client/api/updater_util.py, line 108 > > <https://reviews.apache.org/r/30461/diff/2/?file=842714#file842714line108> > > > > Can you investigate whether the python code respects the `isSetX` > > pattern? My hunch is that it does, and we should leverage it to > > distinguish default `0` from the user specifying `0`.
This is coming from the schema as `Default(Integer, 0)`. Dropped `Default` completely. > On Feb. 3, 2015, 12:28 a.m., Bill Farner wrote: > > src/main/python/apache/aurora/client/api/updater_util.py, line 25 > > <https://reviews.apache.org/r/30461/diff/2/?file=842714#file842714line25> > > > > I suggest increasing this, say to 120. We already have a pretty good > > safety net built in, i wouldn't want to start battling micro-outages from > > the beginning. > > > > Also, a comment would be useful here to fill in context about what this > > is, what are the effects of a high/low value. > > Joshua Cohen wrote: > This should probably be renamed to, say, MINUMUM_PULSE_INTERVAL_SECONDS? > It doesn't seem to be used as a default anywhere that I see. > > Given that it's a minimum value, I think 120 seconds is probably too > high. Maybe 60? > > Bill Farner wrote: > Oh, if it's a minimum i vote for enforcing it in the scheduler. We don't usually validate ranges for user-defined values on the scheduler. E.g. [1]. I think scheduler should have its own limits enforced when we officially support thrift/REST level job update creations. Until then I'd rather stay consistent with the current approach as having schema variable names (`pulse_interval_secs`) instead of scheduler internal thrift schema names (`blockIfNoPulsesAfterMs`) is more user friendly. [1] - https://github.com/apache/incubator-aurora/blob/3c3b04fd631e6c05e677b298d2c3fd9c5a81ca7d/src/main/python/apache/aurora/client/config.py#L80 Renamed the variable and set the min to 60 seconds. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30461/#review70662 ----------------------------------------------------------- On Jan. 30, 2015, 10:31 p.m., Maxim Khutornenko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30461/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2015, 10:31 p.m.) > > > Review request for Aurora, David McLaughlin, Joshua Cohen, and Bill Farner. > > > Bugs: AURORA-1071 > https://issues.apache.org/jira/browse/AURORA-1071 > > > Repository: aurora > > > Description > ------- > > - Adding pulse_interval_secs into client UpdateConfig and validating its range > - Raising an error in client updater for pulse_interval_secs > > > Diffs > ----- > > src/main/python/apache/aurora/client/api/updater.py > 9f91de625f55514530a4f948d7ecdf7b5614b594 > src/main/python/apache/aurora/client/api/updater_util.py > 9d2e893a6ecff0fc48c7944575578443d41ced78 > src/main/python/apache/aurora/config/schema/base.py > e4433d2d47668f59bce169359131284d361bad09 > src/test/python/apache/aurora/client/api/test_api.py > ff1aff2eac391f219bc7c2483a16e35f916a224c > src/test/python/apache/aurora/client/api/test_updater.py > dd3f228c5062d388b4393aa4fd5b60a685bdb3a6 > src/test/python/apache/aurora/client/api/test_updater_util.py > fe3ac49491ca710761632405ac09de0cc0d038a5 > > Diff: https://reviews.apache.org/r/30461/diff/ > > > Testing > ------- > > ./pants test.pytest src/test/python/apache/aurora/client:: > > > Thanks, > > Maxim Khutornenko > >
