[tahoe-dev] patch review

[zooko]

On Jan 2, 2009, at 18:38 PM, Brian Warner wrote:

 > (I'm reviewing patches that landed while I was out on vacation).
 >
 > [3292]: http://allmydata.org/trac/tahoe/changeset/3292 "refactor
 > some common logging behavior into mixins":
 >
 >  the use of self._parentmsgid is suspicious. It looks like two
 >  subsequent calls to log() like:
 >
 >    self.log("foo")
 >    self.log("bar")
 >
 > will wind up with "bar" being the child of "foo", whereas they ought
 > to both be siblings of the same original parent.

If there is an original parent (self._parentmsgid is not None), then
they will be.  I agree that the use of self._parentmsgid is ugly, but
it seemed that this is how our code was using logging -- emitting an
initial message like "startup" and wanting all subsequent messages to
be children of that initial message.

 > [3306]: http://allmydata.org/trac/tahoe/changeset/3306 "util.base32:
 > loosen the precondition forbidding unicode"
 >
 > As mentioned in the "String encoding in Tahoe" thread, I prefer
 > arguments to be of one class only, and functions which accept
 > unicode-or-bytestring make me nervous. Since you reverted the
 > sys.argv conversion in [3311], I'd like to see [3306], [3307], and
 > possibly [3308] reverted too.

Agreed -- fixed in [3364].

 > [3326]: http://allmydata.org/trac/tahoe/changeset/3326 "storage:
 > accept any size shares"
 >
 > As I mentioned on the ticket (#346), this needs a change to
 > StorageServer.VERSION (to increase the advertised share-size limit)
 > before clients will actually be willing to take advantage of larger
 > shares. I'm not sure what value to suggest we advertise: we could
 > use os.vfsstat() to estimate how much space is available (and
 > subtract our reserved value) at startup and advertise that, or we
 > could find a way to ask the system about the largest filesize we can
 > store, or we could pick some arbitrary large number (like 2**64).

Hm.  Reiserfs supports files up to 8 TiB, ZFS up to 16 EiB, NTFS up to
16 EiB.  Ext3's max file size depends on your block size
configuration.  If you want to waste less space per file, you might
set the block size to 1 KiB, in which case the max file size would be
16 GiB.  The default value (at least on Ubuntu, I think), is block
size of 4 KiB, which means file size max of 2 TiB.

At allmydata.com, we don't have any filesystems larger than 1 TB
anyway, since we use only a single SATA disk per filesystem.
(Hopefully in the near future we'll start deploying 1.5 TB disks.)

So, I think we should assume that the filesystem will not impose a
practical limit (hopefully people will upgrade away from ext3 before
they use filesystems larger than 2 TiB, and hopefully they aren't
using the smaller-block-size configurations of ext3) and just
advertise "available space on this server" as the effective share size
limit, as in your first suggestion above.

http://allmydata.org/trac/tahoe/ticket/569 # storage servers should
# advertise large share support

 > Also, the line that does "max_size % 4294967295" is probably a bug,
 > and should be replaced with "max_size % (2**32)".

Good catch!  Fixed in [3366] -- actually I changed it from mod to min:
min(max_size, 4294967295).  I didn't write "2**32-1" because I can't
stand the idea of the storage servers computing a few Python bytecodes
and a few long int operations for every creation of an immutable
share...

Regards,

Zooko