spotify / sparkey

Simple constant key/value storage library, for read-heavy systems with infrequent large bulk inserts.
Apache License 2.0
1.18k stars 81 forks source link

Refuse to create logfile when the name doesn't end in .spl #16

Open nresare opened 10 years ago

nresare commented 10 years ago

Instead of just complaining :)

nailor commented 10 years ago

Just out of curiosity: does the implementation really rely somewhere, in some client lib, that the file ends up in the .spl?

nresare commented 10 years ago

I don't think so. It's more a convenience for new users that the writehash CLI tool expects an .spl. API users are on their own and can call the files whatever they like.

nailor commented 10 years ago

I still kind of liked the warning mode, instead of completely blocking you to write a file if the extension is wrong

spkrka commented 10 years ago

It's somewhat mixed:

I am not sure what approach is best and if we should enforce it everywhere.

nailor commented 10 years ago

But if this is already the behavior in command line, then forcing this makes sense IMO.

What to do with the APIs, I don't know

rohansingh commented 10 years ago

I'm a big fan of being able to name files whatever you want.

spkrka commented 10 years ago

I think I may agree. I think I'll leave the commandline tool to be as permissive as already is, and possibly also extend the Java API to be able to specify explicit filenames instead of following the convention (as soon as someone actually decides they have that need).

The commands that already assume the convention only do so to simplify the usage, if someone strictly wants some other filenames they can write a minimal commandline tool using the python (or c) api.

nresare commented 10 years ago

I'm fine with a warning as well (or the current behavior)

nresare commented 10 years ago

If we decide to go with freedom to the user (sounds nice, doesn't it?) I would propose that we changed writehash to not require .spl.

spkrka commented 10 years ago

Yes, but to do that, the commandline tool needs to take one additional argument to know both the log file and the index file.

Maybe we could add an optional parameter to specify index file, and if that's specified we drop the requirement that the log file ends with .spl

Qix- commented 8 years ago

I'm also in the camp of naming the files whatever you want. It's either enforce it library-wide, though, or don't enforce it at all. As a new Sparkey user, I ran into this as well - I could create a new logfile with whatever name I wanted, but then when I went to write to it it errored out. Was confused for a moment and it seemed less elegant than it probably should.

spkrka commented 8 years ago

This PR is getting a bit stale so we can't merge it regardless, until it's been updated to latest master.

I am not opposed to letting you choose filenames, but we'll need to fix that in all places so it becomes consistent. For backwards compatibility this means we should add more options to the command line for the affected

For sparkey get we need to add sparkey get [-l logfile] indexfile <key> instead of assuming that the log file ends with .spl.

For sparkey writehash we need to add sparkey writehash [-i indexfile] logfile instead of assuming that the index file ends with .spi.

For sparkey rewrite we need to add support for sparkey rewrite --input-index index-file --input-log log-file --output-index index-file --output-log log-file in addition to the existing syntax.

I think this problem only affects the commandline utility, the internals should already be explicit about these things.

I don't have much time at the moment to do this, but if someone else does I am happy to review it.