Move ImmutableShare to backend specific module, add "backend" interface to Storage Server, use twisted's FilePath #1465

New issue

Closed

opened 2011-08-03 05:45:49 +00:00 by zancas · 15 comments

zancas commented

2011-08-03 05:45:49 +00:00

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.

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

zancas added the

labels 2011-08-03 05:45:49 +00:00

zancas added this to the undecided milestone 2011-08-03 05:45:49 +00:00

zancas self-assigned this 2011-08-03 05:45:49 +00:00

zancas commented

2011-08-03 21:15:18 +00:00

Author

Attachment nullpass_Zancas20110803.darcs.patch (88583 bytes) added

Patch bundle. TestServerWithNullBackend passes, and nothing else does

**Attachment** nullpass_Zancas20110803.darcs.patch (88583 bytes) added Patch bundle. [TestServerWithNullBackend](wiki/TestServerWithNullBackend) passes, and nothing else does

nullpass_Zancas20110803.darcs.patch

86 KiB

zancas commented

2011-08-03 21:32:24 +00:00

Author

Attachment ALLpass_Zancas20110803.darcs.patch (132631 bytes) added

Hmmppphh... all tests pass

**Attachment** ALLpass_Zancas20110803.darcs.patch (132631 bytes) added Hmmppphh... all tests pass

ALLpass_Zancas20110803.darcs.patch

130 KiB

zancas commented

2011-08-03 21:35:07 +00:00

Author

Attachment stillALLpass_Zancas20110803.darcs.patch (137808 bytes) added

additional changes unnecessary for passing, but don't cause failures

**Attachment** stillALLpass_Zancas20110803.darcs.patch (137808 bytes) added additional changes unnecessary for passing, but don't cause failures

stillALLpass_Zancas20110803.darcs.patch

135 KiB

zancas removed their assignment 2011-08-03 21:36:26 +00:00

daira was assigned by zancas

2011-08-03 21:36:26 +00:00

daira added

c/code-storage

and removed

c/unknown

labels 2011-08-03 22:07:20 +00:00

daira removed their assignment 2011-08-03 22:07:20 +00:00

daira self-assigned this 2011-08-03 22:07:20 +00:00

daira was unassigned by zooko

2011-08-04 03:35:45 +00:00

zooko self-assigned this 2011-08-04 03:35:45 +00:00

zooko commented

2011-08-04 19:53:09 +00:00

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

Regarding the first patch, [the file test_backends has been added.](http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1465/nullpass_Zancas20110803.darcs.patch#L21), 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](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](http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1465/nullpass_Zancas20110803.darcs.patch#L270) 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](http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1465/nullpass_Zancas20110803.darcs.patch#L293) 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](http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1465/nullpass_Zancas20110803.darcs.patch#L327): good! I'm glad to see this functionality being tested like this. * [TestServerAndFSBackend.test_read_old_share](http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1465/nullpass_Zancas20110803.darcs.patch#L378): 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).

zooko removed their assignment 2011-08-04 19:53:09 +00:00

zancas was assigned by zooko

2011-08-04 19:53:09 +00:00

zancas removed their assignment 2011-08-08 17:24:00 +00:00

zancas self-assigned this 2011-08-08 17:24:00 +00:00

zancas commented

2011-08-10 05:50:57 +00:00

Author

Attachment addresseszookocomment01_20110809.darcs.patch (140448 bytes) added

changes the name of a patch to something more descriptive per zookos comment

**Attachment** addresseszookocomment01_20110809.darcs.patch (140448 bytes) added changes the name of a patch to something more descriptive per zookos comment

addresseszookocomment01_20110809.darcs.patch

137 KiB

zancas commented

2011-08-10 17:54:11 +00:00

Author

Attachment addresseszookocomment02_whitespace_20110810.darcs.patch (147777 bytes) added

whitespace-cleanup run on most files touched by patches

**Attachment** addresseszookocomment02_whitespace_20110810.darcs.patch (147777 bytes) added whitespace-cleanup run on most files touched by patches

addresseszookocomment02_whitespace_20110810.darcs.patch

144 KiB

zancas commented

2011-08-10 20:19:29 +00:00

Author

Attachment addresseszookocomment03_whitespace_pyflakes_20110810.darcs.patch (155358 bytes) added

ugghh... more cleaning to do

**Attachment** addresseszookocomment03_whitespace_pyflakes_20110810.darcs.patch (155358 bytes) added ugghh... more cleaning to do

addresseszookocomment03_whitespace_pyflakes_20110810.darcs.patch

152 KiB

zancas commented

2011-08-10 21:04:17 +00:00

Author

Attachment newseries01_20110810.darcs.patch (50278 bytes) added

Initial file in a new iteration of rational patch-set structuring.

**Attachment** newseries01_20110810.darcs.patch (50278 bytes) added Initial file in a new iteration of rational patch-set structuring.

newseries01_20110810.darcs.patch

49 KiB

zancas commented

2011-08-10 21:26:43 +00:00

Author

Attachment newseries_backendtouched_20110810.darcs.patch (34451 bytes) added

This patch bundle depends on newseries01

**Attachment** newseries_backendtouched_20110810.darcs.patch (34451 bytes) added This patch bundle depends on newseries01

newseries_backendtouched_20110810.darcs.patch

34 KiB

zancas commented

2011-08-10 22:10:44 +00:00

Author

Attachment newseries_peripherals_20110810.darcs.patch (61826 bytes) added

makes changes in storage/common.py, backends/base.py, and allmydata/interfaces.py changes are minimal for passage of "null" test_write_share

**Attachment** newseries_peripherals_20110810.darcs.patch (61826 bytes) added makes changes in storage/common.py, backends/base.py, and allmydata/interfaces.py changes are minimal for passage of "null" test_write_share

newseries_peripherals_20110810.darcs.patch

60 KiB

zancas removed their assignment 2011-08-11 04:41:37 +00:00

zancas self-assigned this 2011-08-11 04:41:37 +00:00

zancas commented

2011-08-29 18:53:38 +00:00

Author

Attachment 20110829dastodisk.darcs.patch (134085 bytes) added

This patch changes the older "das" term to the current "disk" as a backend type.

**Attachment** 20110829dastodisk.darcs.patch (134085 bytes) added This patch changes the older "das" term to the current "disk" as a backend type.

20110829dastodisk.darcs.patch

131 KiB

zancas commented

2011-08-29 18:56:50 +00:00

Author

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.

I added a [dastodisk patch](http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1465/20110829dastodisk.darcs.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.

zancas commented

2011-08-29 21:50:49 +00:00

Author

Attachment 20110829passespyflakes.darcs.patch (137412 bytes) added

pyflaked patch

**Attachment** 20110829passespyflakes.darcs.patch (137412 bytes) added pyflaked patch

20110829passespyflakes.darcs.patch

134 KiB

zancas commented

2011-08-29 21:52:07 +00:00

Author

This patch has all the pyflake-caught cruft cleaned up.

[This patch](http://tahoe-lafs.org/trac/tahoe-lafs/attachment/ticket/1465/20110829passespyflakes.darcs.patch) has all the pyflake-caught cruft cleaned up.

zooko commented

2011-09-02 04:45:44 +00:00

The three things mentioned in this ticket are done in [attachment:20110829passespyflakes.darcs.patch](/tahoe-lafs/trac/attachments/000078ac-4240-b033-f5f2-0a1cec1ec9cc). 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](/tahoe-lafs/trac/attachments/000078ac-4240-b033-f5f2-0a1cec1ec9cc) 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.

The three things mentioned in this ticket are done in [[attachment:20110829passespyflakes.darcs.patch](/tahoe-lafs/trac/attachments/000078ac-4240-b033-f5f2-0a1cec1ec9cc)](/tahoe-lafs/trac/attachments/000078ac-4240-b033-f5f2-0a1cec1ec9cc). 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](/tahoe-lafs/trac/attachments/000078ac-4240-b033-f5f2-0a1cec1ec9cc)](/tahoe-lafs/trac/attachments/000078ac-4240-b033-f5f2-0a1cec1ec9cc) 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](http://twistedmatrix.com/documents/current/api/twisted.python.filepath.FilePath.html) (#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.

Rows
Columns