Opened at 2011-08-03T05:45:49Z
Closed at 2011-09-02T04:45:44Z
#1465 closed enhancement (fixed)
Move ImmutableShare to backend specific module, add "backend" interface to Storage Server, use twisted's FilePath
| Reported by: | Zancas | Owned by: | zancas |
|---|---|---|---|
| Priority: | major | Milestone: | undecided |
| Component: | code-storage | Version: | 1.8.2 |
| Keywords: | review-needed | Cc: | zooko |
| Launchpad Bug: |
Description (last modified by Zancas)
As a first step in implementing a pluggable backend, move ImmutableShare to a module that is specific to the Disk backend. The Disk backend is the backend that corresponds to the current default storage medium. StorageServer no longer communicates directly with the filesystem for purposes of setting up storage, instead it communicates with a backend "core" object that is specific to the type of storage. A Core class lives in each backend specific module along with the corresponding ImmutableShare.
Attachments (11)
Change History (21)
comment:1 Changed at 2011-08-03T05:46:26Z by Zancas
- Description modified (diff)
Changed at 2011-08-03T21:15:18Z by Zancas
Changed at 2011-08-03T21:35:07Z by Zancas
additional changes unnecessary for passing, but don't cause failures
comment:2 Changed at 2011-08-03T21:36:26Z by Zancas
- Keywords review needed added
- Owner changed from Zancas to David-Sarah
comment:3 Changed at 2011-08-03T22:07:20Z by davidsarah
- Cc david-sarah removed
- Component changed from unknown to code-storage
- Keywords review-needed added; review needed removed
- Owner changed from David-Sarah to davidsarah
- Status changed from new to assigned
comment:4 Changed at 2011-08-04T03:35:45Z by zooko
- Owner changed from davidsarah to zooko
- Status changed from assigned to new
comment:5 Changed at 2011-08-04T19:53:09Z by zooko
- Owner changed from zooko to zancas
Regarding the first patch, the file test_backends has been added., here are a few comments.
- Please make the "patch name" more informative to someone who isn't that familiar with the context. See also the suggested patch names on wiki:Patches . For this patch, I would suggest something like "storage: add tests of the new feature of having the storage backend in a separate object from the server".
- Emacs has a command M-x whitespace-cleanup that removes trailing whitespace from lines. Brian and I use it (or its equivalent) traditionally, so if you do too then there will be fewer lines unnecessarily touched. (I don't think David-Sarah uses emacs, but they do somehow always have whitespace-clean line endings...)
- TestServerWithNullBackend.test_write_share needs a docstring explaining what behavior it is making sure happens or doesn't happen in the Code-Under-Test.
- TestServerContruction.test_create_server_fs_backend has such a docstring! But it is out of date. :-) I believe we've changed the test so it is no longer checking whether the Code-Under-Test read or writes outside of its prescribed directory, and instead are merely checking whether it finishes running its constructor without raising an exception.
- TestServerAndFSBackend.test_write_and_read_share: good! I'm glad to see this functionality being tested like this.
- TestServerAndFSBackend.test_read_old_share: Likewise.
- Everything that says "FS" is supposed to be changed to say "Disk", if I understand correctly.
Otherwise this looks like a good patch! I look forward to reviewing the rest of them...
I like the use of a mock/fake filesystem instead of the real local filesystem (disclaimer: I worked with Zancas on doing that for these patches).
comment:6 Changed at 2011-08-08T17:24:00Z by Zancas
- Owner changed from zancas to Zancas
Changed at 2011-08-10T05:50:57Z by Zancas
changes the name of a patch to something more descriptive per zookos comment
Changed at 2011-08-10T21:04:17Z by Zancas
Initial file in a new iteration of rational patch-set structuring.
Changed at 2011-08-10T22:10:44Z by Zancas
makes changes in storage/common.py, backends/base.py, and allmydata/interfaces.py changes are minimal for passage of "null" test_write_share
comment:7 Changed at 2011-08-11T04:41:37Z by Zancas
- Owner changed from Zancas to zancas
Changed at 2011-08-29T18:53:38Z by zancas
This patch changes the older "das" term to the current "disk" as a backend type.
comment:8 Changed at 2011-08-29T18:56:50Z by zancas
I added a dastodisk patch that corrects the backend-type name from "das" to "disk". This patch includes all the changes to the code I've made in the course of implementing a pluggable backend. You should apply it, in preference to any previous patch.
comment:9 Changed at 2011-08-29T21:52:07Z by zancas
This patch has all the pyflake-caught cruft cleaned up.
comment:10 Changed at 2011-09-02T04:45:44Z by zooko
- Resolution set to fixed
- Status changed from new to closed
The three things mentioned in this ticket are done in attachment:20110829passespyflakes.darcs.patch. The patch breaks a bunch of unit tests, but its own allmydata.test.test_backends unit tests pass. The sequence of darcs patches in attachment:20110829passespyflakes.darcs.patch are not nicely rebased to be a few coherent patches with nice descriptions, but the code that they contain is the most current code for #999. The patch doesn't change all code in Tahoe-LAFS to use twisted.python.filepath.FilePath (#1437), but it does consistently use FilePath within the code that it touches.
I'm going to close this ticket as "fixed" and move discussion and new patches back to the over-arching #999.
Patch bundle. TestServerWithNullBackend? passes, and nothing else does