search

ipfs / go-ds-flatfs

A datastore implementation using sharded directories and flat files to store data
MIT License
46 stars 21 forks source link

Use JSON file for `diskUsage.cache` and include note on accuracy of value #35

Closed kevina closed 6 years ago

kevina commented 6 years ago

Alternative to #33.

Closes #31. Closes #37.

Kubuxu commented 6 years ago

Won't this need migration?

Stebalien commented 6 years ago

Won't this need migration?

We haven't bubbled the previous changes yet.

kevina commented 6 years ago

Note. I enhanced the tests and am now getting this failure:

--- FAIL: TestDiskUsage (0.95s)
    --- FAIL: TestDiskUsage/next-to-last (0.32s)
        flatfs_test.go:401: /tmp/test-datastore-flatfs-557319926
        flatfs_test.go:416: duNew: 1238
        flatfs_test.go:433: duPostPut: 4038
        flatfs_test.go:448: duPostDelete: 2038
        flatfs_test.go:451: duFinal: 2038
        flatfs_test.go:470: Unexpected value for diskUsage in diskUsage.cache: 2058 (expected 2038)

I put debug statements in the code and discovered that the value stored in diskUsage.cache is a value from a previous write. I verified that the final write to disk on close is being done in the correct order and has the expected value. Right now my only conclusion is that the o.s. is doing the renaming from the temporary files out of order and an older rename is being done before the final, but I can't seam to find any documentation that this is ever a problem when doing a "write to temp file and rename over the original". I do know that you should probably sync the temporary file before close but I tried that and it didn't seam to help.

I will look more into this tomorrow, but suggestions welcome. Do I need to sync the directory also?

kevina commented 6 years ago

I think many checkpoints are discarded because the channel is full (// checkpoint request already pending). Thus you may be discarding the last one with the expected size in the test because previous ones are pending.

That is the intended behavior, it keeps unnecessary ones from piling up. The final checkpoint is not discarded because it only happens when the channel is closed. I verified this with debug print statements that I didn't push.

kevina commented 6 years ago

I found the bug, I was doing something stupid. This should be ready for review now.

hsanjuan commented 6 years ago

Are we sure this is not overkill in terms of code and complexity just to inform the user that the disk usage is approximate (which always is anyway) ?

I can imagine that either users care about accuracy, in which case, given we cannot tell them how far the approximation is from reality, they'll want to manually calculate and fix their diskUsage files. Or don't care so much, in which case it is the same how the approximation was performed. This might be solving a pure documentation issue with complex code and I'm not sure it's the best thing. Just my two cents...

kevina commented 6 years ago

@hsanjuan this was mostly @Stebalien idea, it also solves some other problems such as doing the checkpoint asynchronously and ensuring the disk usage is written to disk every 2 seconds.

Stebalien commented 6 years ago

@hsanjuan basically, the only complex changes here are performance related. The "use a single json file" change should actually simplify things (although my motivation was to allow us to atomically update the accuracy and the size).