Immutable tiered storage #962

itaiad200 · 2020-11-25T15:01:45Z

Initial draft of the tiered storage.

codecov-io · 2020-11-25T15:12:19Z

Codecov Report

Merging #962 (891e9d4) into master (5e34175) will increase coverage by 0.49%.
The diff coverage is 61.95%.

@@            Coverage Diff             @@
##           master     #962      +/-   ##
==========================================
+ Coverage   44.24%   44.74%   +0.49%     
==========================================
  Files         140      147       +7     
  Lines       11570    11850     +280     
==========================================
+ Hits         5119     5302     +183     
- Misses       5800     5867      +67     
- Partials      651      681      +30

Impacted Files	Coverage Δ
config/config.go	`33.54% <42.85%> (+0.44%)`	⬆️
pyramid/tierFS.go	`59.72% <59.72%> (ø)`
pyramid/ro_file.go	`71.42% <71.42%> (ø)`
pyramid/file.go	`76.47% <76.47%> (ø)`
pyramid/eviction.go	`77.77% <77.77%> (ø)`
graveler/staging.go	`100.00% <0.00%> (ø)`
graveler/graveler.go	`0.00% <0.00%> (ø)`
graveler/staging_iterator.go	`81.08% <0.00%> (ø)`
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e34175...891e9d4. Read the comment docs.

arielshaqed

Nice!

Will use this to store commits.

pyramid/file.go

pyramid/pyramid.go

pyramid/tierFS.go

pyramid/file.go

pyramid/tierFS.go

arielshaqed

Thanks!

Probably need to unit-test everything here; in particular I suspect file creation might not do what we want (comments marked).

config/config.go

pyramid/eviction.go

pyramid/file.go

pyramid/tierFS.go

pyramid/file.go

pyramid/tierFS.go

arielshaqed

Thanks!
Sorry it's taking so long. As discussed the largest change is probably to write files outside the scope of the same files as reading them. Specifically we need to be able to close a to-be-written-to-S3 file without copying it out to S3, e.g. to abort it when Pebble has a write error. And that can even happen when Pebble is closing the file, at which point it commits to close its received file.

config/config.go

pyramid/file.go

pyramid/pyramid.go

pyramid/file.go

pyramid/tierFS.go

pyramid/file.go

pyramid/tierFS.go

arielshaqed

This is really nice, and a lot simpler!

Some issues:

I don't think size-based LRU will make good decisions (I am not familiar with any literature about it either way, though). I think that it always makes cache thrashing worse. This is a criticism of our fork of the LRU cache, I just think it is particularly relevant here in our use-case: most user workloads will have a mix of very large and very small files (say, commits vs. trees), and frequent access to a small file should prefer to displace small files to large ones. We can partly overcome this with multiple pyramids, but then we need to configure each one separately.
Need to delete empty directories from the cache. Otherwise a long-lived system can end up with high fan-out, which is even less efficient on POSIX than on S3.
Can we handle paths with zero-length subdirectories? In S3 the paths s3://foo/a////b and s3://foo/a///b are distinct, whereas in POSIX /a////b, /a///b and /a/b all point to the same place.
If we never use pyramid to cache general user files on disk, then we can just fail pathnames with an empty directory component. Otherwise we will need metadata.

arielshaqed · 2020-12-06T11:27:40Z

pyramid/ro_file.go

+}
+
+func (f *ROFile) Stat() (os.FileInfo, error) {
+	return f.fh.Stat()


I do want to touch here. But... as long as we collect useful metrics from pyramid (not necessarily in this PR!), that's OK either way.

Added a touch - metrics in next PR.

arielshaqed · 2020-12-06T11:30:41Z

pyramid/tierFS.go

 	return nil
 }

 func (tfs *TierFS) removeFromLocal(rPath relativePath) {
-	removeFromLocal(tfs.fsLocalBaseDir, rPath)
+	// This will be called by the cache eviction mechanism during entry insert.
+	// We don't want to wait while the file is being removed from the local disk.


Neat! But it does mean we will need to be very careful how we handle errors there.

Nothing much we can do, right?
For now logging it, will add metrics later on.

pyramid/tierFS.go

arielshaqed · 2020-12-06T11:37:00Z

pyramid/tierFS.go

@@ -320,6 +304,10 @@ func (tfs *TierFS) newLocalFileRef(namespace, filename string) localFileRef {
 }

 func (tfs *TierFS) objPointer(namespace, filename string) block.ObjectPointer {
+	if runtime.GOOS == "windows" {
+		filename = strings.ReplaceAll(filename, `\\'`, "/")


This change is fundamentally unsafe, e.g. if/when MacOS decides it has a new way of writing file paths. I think that in all files that are not local, we should avoid filepath.Join in favour of URL-based joiners or our very own, and write our own /s.

Not sure I follow - how can we prepare for a scenario when MacOS decides to use '^' as the separator? Filenames can contain the new separator.

arielshaqed · 2020-12-06T11:45:40Z

pyramid/file_test.go

+)
+
+func TestPyramidWriteFile(t *testing.T) {
+	filename := uuid.Must(uuid.NewRandom()).String()


Not a fan of UUIDs or of random data in tests, but if we're going to use them then this form is documented as exactly the above:

Suggested change

filename := uuid.Must(uuid.NewRandom()).String()

filename := uuid.New().String()

Changed - need to some randomness so that subsequent runs after a fail test won't look at the same data (if failed to be deleted)

pyramid/file_test.go

itaiad200 · 2020-12-06T13:47:29Z

This is really nice, and a lot simpler!

Some issues:

I don't think size-based LRU will make good decisions (I am not familiar with any literature about it either way, though). I think that it always makes cache thrashing worse. This is a criticism of our fork of the LRU cache, I just think it is particularly relevant here in our use-case: most user workloads will have a mix of very large and very small files (say, commits vs. trees), and frequent access to a small file should prefer to displace small files to large ones. We can partly overcome this with multiple pyramids, but then we need to configure each one separately.

Need to delete empty directories from the cache. Otherwise a long-lived system can end up with high fan-out, which is even less efficient on POSIX than on S3.

Can we handle paths with zero-length subdirectories? In S3 the paths s3://foo/a////b and s3://foo/a///b are distinct, whereas in POSIX /a////b, /a///b and /a/b all point to the same place.
If we never use pyramid to cache general user files on disk, then we can just fail pathnames with an empty directory component. Otherwise we will need metadata.

FS is already per usage (e.g. separate FS for trees and sstables). There are several reasons for this choice and you mentioned some. Other reasons are the need for separation of the disk usage, dynamic FS allocations and more. I also don't think LRU cache is optimised for variable cost entries - we'll have metrics to understand exactly by how much.
I'm making 2 assumptions here, that I'll think will hold for the first version:

Disk capacity per FS is much bigger than a single file (>1000)
Files are roughly the same size.

Done.
Good point. Currently there's no use-case for user based files and we control all paths. Added verification to file paths.

arielshaqed

Great, thanks!

2 small requests for follow on PRs:

Rename tierFS.go to tier_fs.go.
Monitoring. It's a cache, and we've already disagreed about lru.

tzahij · 2020-12-06T12:48:52Z

pyramid/file.go

+	return f.fh.Read(p)
+}
+
+func (f *File) ReadAt(p []byte, off int64) (n int, err error) {


off confused me. please change to offset

I would rather keep the same os.File terminology :)

tzahij · 2020-12-06T12:59:18Z

pyramid/file.go

+	return f.fh.Sync()
+}
+
+func (f *File) Close() error {


I think it will be simpler to use Store instead of Close. Store will close the file.
If someone wants to close without storing - he/she should use of.File

I would love to unify them. But Close may be called by a different layer, e.g. while closing sstable writer.

tzahij · 2020-12-06T13:01:16Z

pyramid/file_test.go

+func TestPyramidWriteFile(t *testing.T) {
+	filename := uuid.Must(uuid.NewRandom()).String()
+	filepath := path.Join("/tmp", filename)
+	defer os.Remove(filepath)


Should be after create

outdated comment.

tzahij · 2020-12-06T13:21:16Z

pyramid/ro_file.go

+
+// ROFile is pyramid wrapper for os.file that implements io.ReadCloser
+// with hooks for updating evictions.
+type ROFile struct {


File should be WOFile

Why? it's also readable.

tzahij · 2020-12-06T13:22:26Z

pyramid/file.go

+)
+
+// File is pyramid wrapper for os.file that triggers pyramid hooks for file actions.
+type File struct {


Where is the file name kept?

Temp file name, which is only relevant during the write itself, is held by the os.File handle.
The final filename is passed during Store.

tzahij · 2020-12-06T14:31:09Z

pyramid/tierFS.go

+type relativePath string
+
+// localFileRef consists of all possible local file references
+type localFileRef struct {


I suggest that the two last properties will be methods on localFileRef

Good point. But then I'll have to keep a pointer to tfs, which is a bit messy.
I don't feel strongly about it, willing to change if you want.

itaiad200 requested review from tzahij and arielshaqed November 25, 2020 15:01

arielshaqed reviewed Nov 25, 2020

View reviewed changes

itaiad200 force-pushed the feature/tiered-storage branch from 5b8f6b0 to b353aae Compare November 26, 2020 15:52

First version of the immutable tiered storage

856e506

itaiad200 force-pushed the feature/tiered-storage branch from b353aae to 856e506 Compare November 29, 2020 16:16

itaiad200 added 3 commits November 30, 2020 11:15

Merge remote-tracking branch 'origin/master' into feature/tiered-storage

36ae2c7

Add ristretto cache for eviction control

94eab34

Handle file refs better, add startup logic

b9ac0b4

itaiad200 force-pushed the feature/tiered-storage branch from 99cb63e to b9ac0b4 Compare November 30, 2020 16:54

Merge remote-tracking branch 'origin/master' into feature/tiered-storage

121283c

arielshaqed reviewed Dec 1, 2020

View reviewed changes

PR changes

77ed7cb

itaiad200 force-pushed the feature/tiered-storage branch from f26f5b2 to 77ed7cb Compare December 1, 2020 14:58

add tests

e8b8c87

itaiad200 changed the title ~~First version of the immutable tiered storage~~ Immutable tiered storage Dec 2, 2020

itaiad200 requested a review from arielshaqed December 2, 2020 12:45

itaiad200 marked this pull request as ready for review December 2, 2020 12:45

Merge remote-tracking branch 'origin/master' into feature/tiered-storage

21c6826

itaiad200 force-pushed the feature/tiered-storage branch from add7902 to 21c6826 Compare December 2, 2020 12:49

itaiad200 requested a review from johnnyaug December 2, 2020 12:50

Add tests for pyramid file

fe134d8

itaiad200 force-pushed the feature/tiered-storage branch from 834af95 to fe134d8 Compare December 2, 2020 15:00

arielshaqed reviewed Dec 2, 2020

View reviewed changes

itaiad200 added 2 commits December 3, 2020 16:30

Replace cache instance

207d733

Merge remote-tracking branch 'origin/master' into feature/tiered-storage

e41a8a8

itaiad200 requested a review from arielshaqed December 3, 2020 14:53

windows aware paths

9acd925

itaiad200 force-pushed the feature/tiered-storage branch from 0f4617f to 9acd925 Compare December 3, 2020 15:04

itaiad200 added 2 commits December 3, 2020 17:17

Add paralalism for cache eviction hook

2eedc44

Merge remote-tracking branch 'origin/master' into feature/tiered-storage

93dbd1e

arielshaqed requested changes Dec 6, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into feature/tiered-storage

d7da950

Fixing PR comments

891e9d4

itaiad200 requested a review from arielshaqed December 6, 2020 14:08

arielshaqed approved these changes Dec 6, 2020

View reviewed changes

itaiad200 merged commit e88f3c7 into master Dec 6, 2020

tzahij reviewed Dec 6, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Immutable tiered storage #962

Immutable tiered storage #962

itaiad200 commented Nov 25, 2020

codecov-io commented Nov 25, 2020 •

edited

Loading

arielshaqed left a comment

arielshaqed left a comment

arielshaqed left a comment

arielshaqed left a comment

arielshaqed Dec 6, 2020

itaiad200 Dec 6, 2020

arielshaqed Dec 6, 2020

itaiad200 Dec 6, 2020

arielshaqed Dec 6, 2020

itaiad200 Dec 6, 2020

arielshaqed Dec 6, 2020

itaiad200 Dec 6, 2020

itaiad200 commented Dec 6, 2020

arielshaqed left a comment

tzahij Dec 6, 2020

itaiad200 Dec 6, 2020

tzahij Dec 6, 2020

itaiad200 Dec 6, 2020

tzahij Dec 6, 2020

itaiad200 Dec 6, 2020

tzahij Dec 6, 2020

itaiad200 Dec 6, 2020

tzahij Dec 6, 2020

itaiad200 Dec 6, 2020

tzahij Dec 6, 2020

itaiad200 Dec 6, 2020

	filename := uuid.Must(uuid.NewRandom()).String()
	filename := uuid.New().String()

Immutable tiered storage #962

Immutable tiered storage #962

Conversation

itaiad200 commented Nov 25, 2020

codecov-io commented Nov 25, 2020 • edited Loading

Codecov Report

arielshaqed left a comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaiad200 commented Dec 6, 2020

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 25, 2020 •

edited

Loading