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

Option to store buffer data #5

Closed
wants to merge 3 commits into from
Closed

Option to store buffer data #5

wants to merge 3 commits into from

Conversation

@avitale
Copy link
Contributor

avitale commented Jun 17, 2014

This pull request adds the option to store buffer data in catbox-memory.
With this option catbox-memory can be used to store pre-compressed data (ex gzip).

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Aug 20, 2014

Need to rebase against master. Also you should probably update the README explaining this new option.

@arb

This comment has been minimized.

Copy link
Contributor

arb commented Sep 4, 2014

Ping @avitale. This PR is out of date and we haven't heard anything from you in quite a while.

@geek geek added the enhancement label Sep 5, 2014
}
catch (err) {
return callback(err);
if(this.settings.buffer){

This comment has been minimized.

Copy link
@geek

geek Sep 5, 2014

Member

if (this.settings.buffer) {

if(this.settings.buffer){
if (Buffer.isBuffer(value)) {
stringifiedValue = value;
} else {

This comment has been minimized.

Copy link
@geek

geek Sep 5, 2014

Member

else on new line

@avitale

This comment has been minimized.

Copy link
Contributor Author

avitale commented Sep 5, 2014

Hi @arb, if you are interested in merging this pull request I can look into it again

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Sep 5, 2014

@avitale yes, please resolve the merge conflicts.

}
catch (err) {
return callback(new Error('Bad value content'));
if(this.settings.buffer){

This comment has been minimized.

Copy link
@cjihrig

cjihrig Sep 5, 2014

Contributor

Please add a space before ( and after )

return callback(new Error('Bad value content'));
if(this.settings.buffer){
value = envelope.item;
} else {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Sep 5, 2014

Contributor

else on new line

README.md Outdated
@@ -10,4 +10,4 @@ Memory adapter for catbox
cached. Once this limit is reached no additional items will be added to the cache
until some expire. The utilized memory calculation is a rough approximation and must
not be relied on. Defaults to `104857600` (100MB).

- `buffer` - store Buffer data, values are not stringified and parsed. Defaults to `false`.

This comment has been minimized.

Copy link
@cjihrig

cjihrig Sep 5, 2014

Contributor

Could you add a sentence mentioning that storing a Buffer means that the contents could change while in cache.

@@ -73,6 +73,42 @@ describe('Memory', function () {
});
});

it('gets a buffer item after setting it', function (done) {

var buffer = new Buffer("I'm a string!", "utf-8");

This comment has been minimized.

Copy link
@cjihrig

cjihrig Sep 5, 2014

Contributor

Please use single quotes throughout the code.


var buffer = new Buffer("I'm a string!", "utf-8");

var client = new Catbox.Client(new Memory({buffer: true}));

This comment has been minimized.

Copy link
@cjihrig

cjihrig Sep 5, 2014

Contributor

No blank line before this. Add a blank line after. Add a space after { and before }

var client = new Catbox.Client(new Memory({buffer: true}));
client.start(function (err) {

var key = { id: 'x', segment: 'test' };

This comment has been minimized.

Copy link
@cjihrig

cjihrig Sep 5, 2014

Contributor

Blank line following this one.


it('fails setting a non buffer when expecting a buffer', function (done) {

var client = new Catbox.Client(new Memory({buffer: true}));

This comment has been minimized.

Copy link
@cjihrig

cjihrig Sep 5, 2014

Contributor

New line following this. Spaces around curly braces.

var client = new Catbox.Client(new Memory({buffer: true}));
client.start(function (err) {

var key = { id: 'x', segment: 'test' };

This comment has been minimized.

Copy link
@cjihrig

cjihrig Sep 5, 2014

Contributor

Please add a new line.

client.start(function (err) {

var key = { id: 'x', segment: 'test' };
client.set(key, "I'm a string!", 500, function (err) {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Sep 5, 2014

Contributor

Please use single quotes.


var key = { id: 'x', segment: 'test' };
client.set(key, "I'm a string!", 500, function (err) {
expect(err.message).to.equal('Value is not a Buffer');

This comment has been minimized.

Copy link
@cjihrig

cjihrig Sep 5, 2014

Contributor

Blank line at the beginning of a new function.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Sep 5, 2014

This is good, but I think it would be better to support a mix of Buffers and stringified objects.

@avitale

This comment has been minimized.

Copy link
Contributor Author

avitale commented Sep 6, 2014

Supporting a mix would be great. I am wondering if we can do it in catbox-memory or catbox API has to be changed as well. Any opinion about it?

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Sep 8, 2014

I was thinking you could add a flag allowMixedContent. If it's set to true (default is false) and a Buffer instance is passed in, then use the implementation you've built. Otherwise, use the existing JSON.stringify() approach.

@avitale

This comment has been minimized.

Copy link
Contributor Author

avitale commented Sep 8, 2014

I have improved the code, now it supports mixed content. I haven't yet added the allowMixedContent flag, what is it's purpose? Do you think that storing buffers properly breaks backward compatibility?

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Sep 8, 2014

Yes, it can when you store a Buffer. First, you are storing a reference to the original object, instead of a stringified copy, meaning that existing code could be modifying the cached value directly. Second, retrieving an item could now return a Buffer instead of the result of JSON.parse(). By using a flag, you're explicitly opting into this different behavior.

@avitale

This comment has been minimized.

Copy link
Contributor Author

avitale commented Sep 9, 2014

Thanks for the useful comments, I have added the allowMixedContent and changed the code so that buffers are copied before storing, this way the behavior is consistent and predictable.

@cjihrig cjihrig mentioned this pull request Sep 9, 2014
@cjihrig cjihrig added this to the 2.0.0 milestone Sep 9, 2014
@cjihrig cjihrig self-assigned this Sep 9, 2014
@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Sep 9, 2014

@avitale thanks - opened #15 with your commits and a little cleanup

@cjihrig cjihrig closed this Sep 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.