mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [mesos] cf-natali commented on pull request #386: Fixed a LevelDBStorage bug where positions would overflow.
Date Fri, 21 May 2021 19:00:10 GMT

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:
users@infra.apache.org



Mime
View raw message