Added msgpack serializer support #55

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
5 participants
@onethumb
Contributor

onethumb commented Jan 11, 2013

We needed a better compact, binary serializer for memcached that worked across non-PHP languages. msgpack has proven to be very fast and compact and results in significant savings.

See: http://msgpack.org/ & https://github.com/msgpack/msgpack-php/

While I was in there, I fixed the igbinary support, which silently said it was working but actually fell back to the default for serialization due to a typo (MEMCACHE instead of MEMCACHED). :)

@onethumb

This comment has been minimized.

Show comment Hide comment
@onethumb

onethumb Jan 11, 2013

Contributor

I left igbinary as the default if it's enabled, but personally think msgpack would be a saner default if it's enabled given the broader adoption, space savings, etc. Your call. :)

Contributor

onethumb commented Jan 11, 2013

I left igbinary as the default if it's enabled, but personally think msgpack would be a saner default if it's enabled given the broader adoption, space savings, etc. Your call. :)

@andreiz

This comment has been minimized.

Show comment Hide comment
@andreiz

andreiz Jan 28, 2013

Typo, should be HAVE_MEMCACHED_MSGPACK

Typo, should be HAVE_MEMCACHED_MSGPACK

This comment has been minimized.

Show comment Hide comment
@onethumb

onethumb Jan 28, 2013

Owner

Fixing! :)

Owner

onethumb replied Jan 28, 2013

Fixing! :)

iliaal added a commit that referenced this pull request Mar 3, 2013

@staabm

This comment has been minimized.

Show comment Hide comment
@staabm

staabm Apr 27, 2013

Would be great to have this merged

staabm commented Apr 27, 2013

Would be great to have this merged

;
-; The default is igbinary if available and php otherwise.
+; The default is igbinary if available, then msgpack if available, then php otherwise.

This comment has been minimized.

Show comment Hide comment
@onethumb

onethumb Aug 7, 2013

Contributor

Probably need to double-check all the use cases for serialization, make sure msgpack handles them well. I seem to recall some bumps using JSON encoding certain types of objects (maybe just ones with binary contents?)

@onethumb

onethumb Aug 7, 2013

Contributor

Probably need to double-check all the use cases for serialization, make sure msgpack handles them well. I seem to recall some bumps using JSON encoding certain types of objects (maybe just ones with binary contents?)

This comment has been minimized.

Show comment Hide comment
@mkoppanen

mkoppanen Oct 23, 2013

Member

Can you write some tests for the use-cases?

@mkoppanen

mkoppanen Oct 23, 2013

Member

Can you write some tests for the use-cases?

@@ -264,7 +269,7 @@ static PHP_INI_MH(OnUpdateSerializer)
MEMC_G(serializer) = SERIALIZER_DEFAULT;
} else if (!strcmp(new_value, "php")) {
MEMC_G(serializer) = SERIALIZER_PHP;
-#ifdef HAVE_MEMCACHE_IGBINARY
+#ifdef HAVE_MEMCACHED_IGBINARY

This comment has been minimized.

Show comment Hide comment
@onethumb

onethumb Aug 7, 2013

Contributor

Here's where the existing typo disabled igbinary even if you had it configured, I believe.

@onethumb

onethumb Aug 7, 2013

Contributor

Here's where the existing typo disabled igbinary even if you had it configured, I believe.

@davidsteinsland

This comment has been minimized.

Show comment Hide comment
@davidsteinsland

davidsteinsland Oct 22, 2013

Time to merge this in, ey?

Time to merge this in, ey?

@mkoppanen

This comment has been minimized.

Show comment Hide comment
@mkoppanen

mkoppanen Oct 23, 2013

Member

I haven't yet looked at this, let me review. (First glance, don't see any tests)

Member

mkoppanen commented Oct 23, 2013

I haven't yet looked at this, let me review. (First glance, don't see any tests)

php_memcached.c
+#ifdef HAVE_MEMCACHED_MSGPACK
+ case SERIALIZER_MSGPACK:
+ php_msgpack_serialize(&buf, value TSRMLS_CC);
+ if(!buf.c) {

This comment has been minimized.

Show comment Hide comment
@mkoppanen

mkoppanen Oct 23, 2013

Member

Spaces for indentation?

@mkoppanen

mkoppanen Oct 23, 2013

Member

Spaces for indentation?

php_memcached.c
@@ -3019,6 +3048,15 @@ static int php_memc_zval_from_payload(zval *value, char *payload, size_t payload
#endif
break;
+ case MEMC_VAL_IS_MSGPACK:
+#ifdef HAVE_MEMCACHED_MSGPACK
+ php_msgpack_unserialize(value, payload, payload_len TSRMLS_CC);

This comment has been minimized.

Show comment Hide comment
@mkoppanen

mkoppanen Oct 23, 2013

Member

Indentation?

@mkoppanen

mkoppanen Oct 23, 2013

Member

Indentation?

@mkoppanen

This comment has been minimized.

Show comment Hide comment
@mkoppanen

mkoppanen Oct 23, 2013

Member

Added some comments

Member

mkoppanen commented Oct 23, 2013

Added some comments

@mkoppanen

This comment has been minimized.

Show comment Hide comment
@mkoppanen

mkoppanen Nov 17, 2013

Member

Can someone write a few tests for this PR?

Member

mkoppanen commented Nov 17, 2013

Can someone write a few tests for this PR?

@davidsteinsland

This comment has been minimized.

Show comment Hide comment
@davidsteinsland

davidsteinsland Nov 17, 2013

See onethumb#1 ...

@mkoppanen

This comment has been minimized.

Show comment Hide comment
@mkoppanen

mkoppanen Nov 17, 2013

Member

@davidsteinsland,

looks good. If @onethumb can merge that to his I'll merge this PR in one go.

Member

mkoppanen commented Nov 17, 2013

@davidsteinsland,

looks good. If @onethumb can merge that to his I'll merge this PR in one go.

Merge pull request #1 from davidsteinsland/master
Added tests and fixes whitespaces
@davidsteinsland

This comment has been minimized.

Show comment Hide comment
@davidsteinsland

davidsteinsland Nov 18, 2013

Ready to merge.

Ready to merge.

@mkoppanen

This comment has been minimized.

Show comment Hide comment
@mkoppanen

mkoppanen Nov 18, 2013

Member

There is a small issue with msgpack extension. See issue 23 in php-msgpack repo. I'll wait for the response for that to know what to with tests. On super slow edge connection but I'll merge this in the morning at the office.

Member

mkoppanen commented Nov 18, 2013

There is a small issue with msgpack extension. See issue 23 in php-msgpack repo. I'll wait for the response for that to know what to with tests. On super slow edge connection but I'll merge this in the morning at the office.

@mkoppanen

This comment has been minimized.

Show comment Hide comment
@mkoppanen

mkoppanen Nov 19, 2013

Member

Merged msgpack support, this wouldn't merge so had to do some changes. I'll merge the test manually. Thanks guys, and sorry for the wait!

Member

mkoppanen commented Nov 19, 2013

Merged msgpack support, this wouldn't merge so had to do some changes. I'll merge the test manually. Thanks guys, and sorry for the wait!

@mkoppanen mkoppanen closed this Nov 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment