cf-natali commented on pull request #386: URL: https://github.com/apache/mesos/pull/386#issuecomment-846176591
Hey, @bmahler , thanks for looking at this. Yes this commit is more a call for discussion. This change is backward compatible, however will only increase the maximum record position from `INT32_MAX` to `9'999'999'999`, which was the implicit limit when the encoding was chosen. The reason I went for this simple change are that: - it's simple - it's backward compatible - it's not clear how many users are affected by this overflow problem: I don't think Mesos itself should be affected since it'd be quite hard to reach `INT32_MAX` records just with the registry. The original bug report open by @ipronin is quite terse so I'm not sure in which context this was observed - I assume maybe because of a framework used at Twitter which uses the replicated log which managed to overflow it? Trying to extend the maximum position to `UINT64_MAX` is indeed more tricky, especially in a backward compatible way. In particular, I don't think it's possible to recover in an overflow happened if the leveldb segments have been compacted. And if overflow occurred I think it's pretty much all bets off so the only reasonable thing to do is to start clean - repairing would be quite tricky So I can see at least two options: 1. Merge this change, which increases the range by a factor of over 4X, while being simple and backward compatible. 2. Extend the range to `UINT64_MAX` using an approach similar to the one from @ipronin 's patch. However requiring users to use a dedicated tool to update the existing log is IMO not a reasonable approach in general. A compromise would be to do the change described in 1 (this merge) for existing logs, but start using the new 20-chars wide format supporting up to `UINT64_MAX` for new logs. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
