#993 closed enhancement (fixed)

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

Reported by: davidsarah Owned by: kevan
Priority: major Milestone: 1.9.0
Component: code-mutable Version: 1.6.0
Keywords: cleanup download mutable Cc:
Launchpad Bug:


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 src/allmydata/interfaces.py.)

Attachments (1)

interface-changes.py (21.1 KB) - added by davidsarah at 2010-03-12T06:01:13Z.
Proposed refactoring of file download interfaces

Download all attachments as: .zip

Change History (20)

Changed at 2010-03-12T06:01:13Z by davidsarah

Proposed refactoring of file download interfaces

comment:1 Changed at 2010-03-12T06:56:27Z by davidsarah

  • Keywords review-needed added
  • Owner set to warner

comment:2 Changed at 2010-05-03T16:04:47Z by davidsarah

  • Milestone changed from 1.7.0 to 1.8.0

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

comment:3 Changed at 2010-05-03T20:27:55Z by zooko

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.)

comment:4 Changed at 2010-05-04T03:55:36Z by davidsarah

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

comment:5 Changed at 2010-07-07T00:10:59Z by davidsarah

Here 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)
            return None
        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)

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)

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

comment:6 Changed at 2010-07-10T01:13:46Z by davidsarah

  • Owner changed from warner to kevan

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.

comment:7 Changed at 2010-07-17T03:43:28Z by davidsarah

  • Milestone changed from 1.8.0 to 1.8β

comment:8 Changed at 2010-07-23T05:47:16Z by zooko

  • Owner changed from kevan to nobody

comment:9 Changed at 2010-09-05T00:26:20Z by davidsarah

  • Milestone changed from 1.8β to 1.9.0

comment:10 Changed at 2011-01-08T14:22:20Z by zooko

  • Owner changed from nobody to kevan

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.

comment:11 Changed at 2011-01-08T22:50:41Z by kevan

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.)

comment:12 Changed at 2011-01-08T22:51:25Z by kevan

  • Keywords review-needed removed

comment:13 Changed at 2011-07-16T21:11:15Z by kevan

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

comment:14 Changed at 2011-07-24T15:19:25Z by davidsarah

In ticket:413#comment:5, 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.)

comment:15 Changed at 2011-10-13T17:04:38Z by warner

  • Milestone changed from 1.9.0 to 1.10.0

not making it into 1.9

comment:16 Changed at 2011-10-18T23:05:19Z by davidsarah

  • Milestone changed from 1.10.0 to 1.9.0
  • Version changed from 1.6.0 to 1.6.1

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

comment:17 Changed at 2011-10-18T23:05:57Z by davidsarah

  • Version changed from 1.6.1 to 1.6.0

Don't know why the Version field changed.

comment:18 Changed at 2011-10-28T04:49:32Z by warner

does that mean we can close this?

comment:19 Changed at 2011-10-28T05:24:22Z by warner

  • Resolution set to fixed
  • Status changed from new to closed

davidsarah said yes. Yay!

Note: See TracTickets for help on using tickets.