refactor download interfaces to treat immutable files and mutable versions more uniformly #993

New issue

Closed

opened 2010-03-12 05:59:45 +00:00 by daira · 15 comments

daira commented

2010-03-12 05:59:45 +00:00

Currently, downloading a mutable file is quite complicated unless the download_best_version() method of IMutableFileNode is used. This has several disadvantages:

client code has to fork depending on whether it is downloading a mutable or immutable file.
download_best_version() returns a Deferred for the whole contents, it does not allow directing the download to a consumer.
there is the potential for race conditions when the client wants to get several pieces of information about the same version.

The attachment gives some proposed interface changes worked out with Brian on IRC. Among other improvements these would allow calling get_best_readable_version() on any filenode, and then using read to download to a consumer. For the time being mutable files would still be downloaded in a single lump, but clients using these new interfaces would not have to change when we fix that.

('diff' got confused and started interleaving changes to unrelated methods, so compare the attachment with source:src/allmydata/interfaces.py.)

Currently, downloading a mutable file is quite complicated unless the download_best_version() method of IMutableFileNode is used. This has several disadvantages: * client code has to fork depending on whether it is downloading a mutable or immutable file. * download_best_version() returns a Deferred for the whole contents, it does not allow directing the download to a consumer. * there is the potential for race conditions when the client wants to get several pieces of information about the same version. The attachment gives some proposed interface changes worked out with Brian on IRC. Among other improvements these would allow calling `get_best_readable_version()` on any filenode, and then using `read` to download to a consumer. For the time being mutable files would still be downloaded in a single lump, but clients using these new interfaces would not have to change when we fix that. ('diff' got confused and started interleaving changes to unrelated methods, so compare the attachment with source:src/allmydata/interfaces.py.)

daira added the

labels 2010-03-12 05:59:45 +00:00

daira added this to the 1.7.0 milestone 2010-03-12 05:59:45 +00:00

daira commented

2010-03-12 06:01:13 +00:00

Author

Attachment interface-changes.py (21646 bytes) added

Proposed refactoring of file download interfaces

**Attachment** interface-changes.py (21646 bytes) added Proposed refactoring of file download interfaces

interface-changes.py

21 KiB

warner was assigned by daira

2010-03-12 06:56:27 +00:00

daira commented

2010-05-03 16:04:47 +00:00

Author

Too disruptive / not enough time left to do this for 1.7.

daira modified the milestone from 1.7.0 to 1.8.0

2010-05-03 16:04:47 +00:00

zooko commented

2010-05-03 20:27:55 +00:00

(David-Sarah and Brian and I discussed the new-downloader in v1.7 or not on IRC. We agreed that it might be good to plan v1.7 with no new-downloader and that it might be good to delay v1.7 by a few weeks to have a solid, reliable new-downloader.)

Brian: is your new-downloader patch predicated on this patch? If so then we either need to bite the bullet and accept this disruptive refactoring for 1.7, at the risk of either destabilizing or delaying 1.7 release, or else plan to release 1.7 without new-downloader. (David-Sarah and Brian and I discussed the new-downloader in v1.7 or not on IRC. We agreed that it might be good to plan v1.7 with no new-downloader and that it might be good to delay v1.7 by a few weeks to have a solid, reliable new-downloader.)

daira commented

2010-05-04 03:55:36 +00:00

Author

Note that the patch attached to this ticket is only a proposed API, not an implementation of it.

daira commented

2010-07-07 00:10:59 +00:00

Author

[Here]source:src/allmydata/frontends/sftpd.py@4521#L706 is an example of code that forks depending on whether it's downloading a mutable or immutable file, in the SFTP frontend:

    # TODO: use download interface described in #993 when implemented.
    if filenode.is_mutable():
        self.async.addCallback(lambda ign: filenode.download_best_version())
        def _downloaded(data):
            self.consumer = OverwriteableFileConsumer(len(data), tempfile_maker)
            self.consumer.write(data)
            self.consumer.finish()
            return None
        self.async.addCallback(_downloaded)
    else:
        download_size = filenode.get_size()
        assert download_size is not None, "download_size is None"
        self.consumer = OverwriteableFileConsumer(download_size, tempfile_maker)
        def _read(ign):
            if noisy: self.log("_read immutable", level=NOISY)
            filenode.read(self.consumer, 0, None)
        self.async.addCallback(_read)

With the proposed interface it could be:

    self.async.addCallback(lambda ign: filenode.get_best_version())
    def _got_best_version(version):
        if noisy: self.log("_got_best_version(%r)" % (version,), level=NOISY)
        download_size = version.get_size()
        assert download_size is not None, "download_size is None"
        self.consumer = OverwriteableFileConsumer(download_size, tempfile_maker)
        version.read(self.consumer, 0, None)
    self.async.addCallback(_got_best_version)

Since the new downloader is milestone 1.8, this can wait until after 1.7.1, though.

[Here]source:src/allmydata/frontends/sftpd.py@4521#L706 is an example of code that forks depending on whether it's downloading a mutable or immutable file, in the SFTP frontend: ``` # TODO: use download interface described in #993 when implemented. if filenode.is_mutable(): self.async.addCallback(lambda ign: filenode.download_best_version()) def _downloaded(data): self.consumer = OverwriteableFileConsumer(len(data), tempfile_maker) self.consumer.write(data) self.consumer.finish() return None self.async.addCallback(_downloaded) else: download_size = filenode.get_size() assert download_size is not None, "download_size is None" self.consumer = OverwriteableFileConsumer(download_size, tempfile_maker) def _read(ign): if noisy: self.log("_read immutable", level=NOISY) filenode.read(self.consumer, 0, None) self.async.addCallback(_read) ``` With the proposed interface it could be: ``` self.async.addCallback(lambda ign: filenode.get_best_version()) def _got_best_version(version): if noisy: self.log("_got_best_version(%r)" % (version,), level=NOISY) download_size = version.get_size() assert download_size is not None, "download_size is None" self.consumer = OverwriteableFileConsumer(download_size, tempfile_maker) version.read(self.consumer, 0, None) self.async.addCallback(_got_best_version) ``` Since the new downloader is milestone 1.8, this can wait until after 1.7.1, though.

daira commented

2010-07-10 01:13:46 +00:00

Author

Kevan is working on this in the patches for #393 (MDMF). Some comments on those patches relating to this ticket:

class IMutableFileVersion(IReadable):
    # TODO: Can this be overwrite instead of replace?
    def replace(new_contents):

Yes, it can be, and should take an IMutableUploadable rather than a bytestring.

class IReadable(Interface):
    def is_readonly():
        """Return True if this reference provides mutable access to the given
        file or directory (i.e. if you can modify it), or False if not. Note
        that even if this reference is read-only, someone else may hold a
        read-write reference to it.

        For an IReadable returned by get_best_readable_version(), this will
        always return True, but for instances of subinterfaces such as
        IMutableFileVersion, it may return False."""

The second paragraph here could be misread; perhaps change it to

        For an IReadable returned by get_best_readable_version(), this will
        always return True. For instances of IMutableFileVersion (a subinterface
        of IReadable) that are returned by get_best_mutable_version(), it can
        return True or False.

Kevan is working on this in the patches for #393 (MDMF). Some comments on those patches relating to this ticket: ``` class IMutableFileVersion(IReadable): # TODO: Can this be overwrite instead of replace? def replace(new_contents): ``` Yes, it can be, and should take an `IMutableUploadable` rather than a bytestring. ``` class IReadable(Interface): def is_readonly(): """Return True if this reference provides mutable access to the given file or directory (i.e. if you can modify it), or False if not. Note that even if this reference is read-only, someone else may hold a read-write reference to it. For an IReadable returned by get_best_readable_version(), this will always return True, but for instances of subinterfaces such as IMutableFileVersion, it may return False.""" ``` The second paragraph here could be misread; perhaps change it to ``` For an IReadable returned by get_best_readable_version(), this will always return True. For instances of IMutableFileVersion (a subinterface of IReadable) that are returned by get_best_mutable_version(), it can return True or False. ```

daira modified the milestone from 1.8.0 to 1.8β

2010-07-17 03:43:28 +00:00

daira modified the milestone from 1.8β to 1.9.0

2010-09-05 00:26:20 +00:00

zooko commented

2011-01-08 14:22:20 +00:00

How can we move this ticket out of review-needed state? I know! Kevan could review it and we could apply the patch to trunk! Kevan: interested? If we're not going to "apply the patch to trunk" because it is only interface changes and not implementation then let's remove the review-needed tag until we have an implementation.

How can we move this ticket out of `review-needed` state? I know! Kevan could review it and we could apply the patch to trunk! Kevan: interested? If we're not going to "apply the patch to trunk" because it is only interface changes and not implementation then let's remove the `review-needed` tag until we have an implementation.

kevan commented

2011-01-08 22:50:41 +00:00

Owner

(Maybe we should make a trac keyword that says that a ticket is in particular need of design feedback or review but doesn't involve any code changes. Then 'review-needed' could be used only for code changes needing a review before they can be committed, and the other keyword could be used for tickets like #993, which don't have a patch that we'd want to apply but could benefit from other perspectives.)

An implementation of this ticket (including the interface) is in #393, so I don't think there's anything to apply here. I'll remove review-needed. I'm tempted to say that #993 could be marked as a duplicate of #393, but it might be useful to be able to discuss the interface independently of #393 as a whole, which is fairly long and involves other issues. (Maybe we should make a trac keyword that says that a ticket is in particular need of design feedback or review but doesn't involve any code changes. Then 'review-needed' could be used only for code changes needing a review before they can be committed, and the other keyword could be used for tickets like #993, which don't have a patch that we'd want to apply but could benefit from other perspectives.)

kevan commented

2011-07-16 21:11:15 +00:00

Owner

This is still addressed by #393, so it should stay in the 1.9.0 milestone.

daira commented

2011-07-24 15:19:25 +00:00

Author

In /tahoe-lafs/trac/issues/26357#comment:377128, zooko said:

"roothash" means the root of the Merkle Tree over the (encrypted) shares of one specific version of the file, so it is sufficient to uniquely identify one version. Yes, everything that talks about "versions" of mutable files should specify the version as a (sequence_number, roothash) tuple.

This affects the new IMutableFileversion interface in interfaces.py: it should either grow a get_roothash() method, or get_sequence_number() should be changed to get_sequence_number_and_roothash() (or some more concise name) that returns a pair.

(The object represents a particular version, so a single method returning a pair is not necessary to avoid race conditions.)

In [/tahoe-lafs/trac/issues/26357](/tahoe-lafs/trac/issues/26357)#[comment:377128](/tahoe-lafs/trac/issues/993#issuecomment-377128), zooko said: > "roothash" means the root of the Merkle Tree over the (encrypted) shares of one specific version of the file, so it is sufficient to uniquely identify one version. Yes, everything that talks about "versions" of mutable files should specify the version as a `(sequence_number, roothash)` tuple. This affects the new `IMutableFileversion` interface in `interfaces.py`: it should either grow a `get_roothash()` method, or `get_sequence_number()` should be changed to `get_sequence_number_and_roothash()` (or some more concise name) that returns a pair. (The object represents a particular version, so a single method returning a pair is not necessary to avoid race conditions.)

warner commented

2011-10-13 17:04:38 +00:00

not making it into 1.9

warner modified the milestone from 1.9.0 to 1.10.0

2011-10-13 17:04:38 +00:00

daira commented

2011-10-18 23:05:19 +00:00

Author

The main changes suggested by this ticket were part of #393, and are present in 1.9. (comment:377135 isn't implemented, but that isn't critical.)

The main changes suggested by this ticket were part of #393, and are present in 1.9. ([comment:377135](/tahoe-lafs/trac/issues/993#issuecomment-377135) isn't implemented, but that isn't critical.)

Rows
Columns