Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

File descriptor flag support #4

Merged

Conversation

marcbachmann
Copy link
Contributor

@marcbachmann marcbachmann commented Jun 28, 2017

Not sure whether it would be simpler passing just a fourth arg 😄
The backwards compatibility is probably also not needed.

This pr

  • changes the arguments to an object
  • adds a flags param
  • adds a cache param

index.js Outdated
var blockSize = opts.blockSize || 1024*16
var codec = opts.codec || {encode: id, decode: id}
var flags = opts.flags || 'a+'
var cache = opts.cache || Cache(1024)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also remove the cache fallback as there is one in the aligned-block-file module

@marcbachmann
Copy link
Contributor Author

I'm currently experimenting with the flags.
Maybe it's better to explicly set read & write mode instead of passing the flags.

@marcbachmann
Copy link
Contributor Author

marcbachmann commented Jun 28, 2017

I wonder why we have to use truncate instead of just resetting the in the blocks file. https://github.com/flumedb/flumelog-offset/blame/master/frame/recoverable.js#L51-L65

That way it would read-only compatible and also restorable from there.

@marcbachmann
Copy link
Contributor Author

The flag itself is really usable. But additionally I was trying to refresh an already opened file. That didn't work because of the truncate and the other process which was writing to the file.

@marcbachmann marcbachmann force-pushed the file-descriptor-flag-support branch from 09f05dc to 442a393 Compare June 30, 2017 22:59
@marcbachmann
Copy link
Contributor Author

I slightly changed the args again to use the filename as first arg and the options as second.
That way it's similar to the api of fs.createReadStream and pull-file

)
module.exports = function (file, opts) {
if (!opts) opts = {}
if (typeof opts !== 'object') legacy(opts, arguments[2])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damn, this is still wrong. Opts could be a codec.

@dominictarr
Copy link
Collaborator

no truncate: #4 (comment)

That is quite a good idea... hmm, it may mean that append mode doesn't work, though. It could just check if it's read only, set the offset and skip truncating. are you expecting to read from a file that is being appended to by someone else? cos that will have other edge cases.

@dominictarr dominictarr merged commit 442a393 into flumedb:master Jul 1, 2017
@dominictarr
Copy link
Collaborator

thanks, merged into 3.1.0

@marcbachmann
Copy link
Contributor Author

marcbachmann commented Jul 1, 2017

Hmm. we still need to update the arg check in to make it completely backwards-compatible. #4 (comment)

typeof obj !== 'object' || obj.decode would do it

are you expecting to read from a file that is being appended to by someone else? cos that will have other edge cases.

Yes. I'm currently writing my own read-only version. I don't need all the caching in the block-file and reads are already optimized. We might need to modularize the module a bit.

e.g.

var file = require('pull-file')
pull(
  file(`${__dirname}/index`, {live: true}),
  pull.map(decode(codec)),
  pull.log()
)

@marcbachmann
Copy link
Contributor Author

marcbachmann commented Jul 1, 2017

I'll write views in a separate process to ensure uptime and reliabilty of the first process.
Views not needed to ensure writes, aka indexes will stay in the main process.

@marcbachmann
Copy link
Contributor Author

Next time I should add a WIP annotation somewhere. 😄
Fix in #5

@marcbachmann
Copy link
Contributor Author

marcbachmann commented Jul 1, 2017

The pull stream version also doesn't work of course because it can't restore if the last block gets written. So I'll use pub/sub on the master process.

Skipping the truncate function would be nice if flags[0] == 'r'.
I'll keep you updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants