Skip to content

Conversation

@sargun
Copy link

@sargun sargun commented Oct 2, 2015

No description provided.

@nickelization
Copy link
Contributor

Looks good! As discussed on HipChat last week, this could theoretically break existing code that uses the kmodify function...but it seems relatively unlikely in practice, and since kmodify is not part of the documented public API either, this seems like a reasonable change to me. Certainly it's a useful feature to have: it would've saved me from having to write all the last_error stuff in data_platform_global_state, and probably could've simplified the leader election service code too.

I would really like to see a simple unit test covering this new code before we merge it though. Doesn't have to be anything fancy, but just something to help avoid future regressions would seem wise, since it's a fairly niche feature that many people might not use or be aware of.

Also, is this intended to be merged directly into 2.0? Since it's a new feature I would maybe expect it to go into develop first, and then naturally make its way into a version branch at some point down the line. I'm kind of fuzzy on our current branching scheme though, so it's possible I'm totally off the wall on this.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants