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

Closed
opened 2010-03-12 05:59:45 +00:00 by davidsarah · 15 comments
davidsarah commented 2010-03-12 05:59:45 +00:00
Owner

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.)
tahoe-lafs added the
code-mutable
major
enhancement
1.6.0
labels 2010-03-12 05:59:45 +00:00
tahoe-lafs added this to the 1.7.0 milestone 2010-03-12 05:59:45 +00:00
davidsarah commented 2010-03-12 06:01:13 +00:00
Author
Owner

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
warner was assigned by tahoe-lafs 2010-03-12 06:56:27 +00:00
davidsarah commented 2010-05-03 16:04:47 +00:00
Author
Owner

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

Too disruptive / not enough time left to do this for 1.7.
tahoe-lafs modified the milestone from 1.7.0 to 1.8.0 2010-05-03 16:04:47 +00:00

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

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.)
davidsarah commented 2010-05-04 03:55:36 +00:00
Author
Owner

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

Note that the patch attached to this ticket is only a proposed API, not an implementation of it.
davidsarah commented 2010-07-07 00:10:59 +00:00
Author
Owner

[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.
davidsarah commented 2010-07-10 01:13:46 +00:00
Author
Owner

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. ```
tahoe-lafs modified the milestone from 1.8.0 to 1.8β 2010-07-17 03:43:28 +00:00
tahoe-lafs modified the milestone from 1.8β to 1.9.0 2010-09-05 00:26: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
Author
Owner

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

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
Author
Owner

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

This is still addressed by #393, so it should stay in the 1.9.0 milestone.
davidsarah commented 2011-07-24 15:19:25 +00:00
Author
Owner

In /tahoe-lafs/trac-2024-07-25/issues/5475#comment:76218, 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-2024-07-25/issues/5475](/tahoe-lafs/trac-2024-07-25/issues/5475)#[comment:76218](/tahoe-lafs/trac-2024-07-25/issues/993#issuecomment-76218), 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.)

not making it into 1.9

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
davidsarah commented 2011-10-18 23:05:19 +00:00
Author
Owner

The main changes suggested by this ticket were part of #393, and are present in 1.9. (comment:76225 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:76225](/tahoe-lafs/trac-2024-07-25/issues/993#issuecomment-76225) isn't implemented, but that isn't critical.)
tahoe-lafs added
1.6.1
and removed
1.6.0
labels 2011-10-18 23:05:19 +00:00
tahoe-lafs modified the milestone from 1.10.0 to 1.9.0 2011-10-18 23:05:19 +00:00
davidsarah commented 2011-10-18 23:05:57 +00:00
Author
Owner

Don't know why the Version field changed.

Don't know why the Version field changed.
tahoe-lafs added
1.6.0
and removed
1.6.1
labels 2011-10-18 23:05:57 +00:00

does that mean we can close this?

does that mean we can close this?

davidsarah said yes. Yay!

davidsarah said yes. Yay!
warner added the
fixed
label 2011-10-28 05:24:22 +00:00
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Reference: tahoe-lafs/trac-2024-07-25#993
No description provided.