codec-memcache: copy metadata in binary full request response #9160

fabienrenaud · 2019-05-19T23:40:26Z

Motivations

Calling copy(), duplicate() or replace() on FullBinaryMemcacheResponse
or FullBinaryMemcacheRequest instances should copy status, opCode, etc.
that are defined in AbstractBinaryMemcacheMessage.

Modifications

Modified duplicate, copy and replace methods in
DefaultFullBinaryMemcacheRequest and DefaultFullBinaryMemcacheResponse
to always copy metadata from parent classes.
Unit tests verifying duplicate, copy and replace methods for
DefaultFullBinaryMemcacheRequest and DefaultFullBinaryMemcacheResponse
copy buffers and metadata as expected.

Result

Calling copy(), duplicate() or replace() methods on
DefaultFullBinaryMemcacheRequest or DefaultFullBinaryMemcacheResponse
produces valid copies with all expected metadata.

Fixes #9159

Checklist:

Signed ICLA

netty-bot · 2019-05-19T23:44:06Z

Can one of the admins verify this patch?

fabienrenaud · 2019-05-19T23:55:14Z

Note: need to add license header and fix line length

On Sun, May 19, 2019 at 16:45 netty-bot ***@***.***> wrote: Can one of the admins verify this patch? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9160?email_source=notifications&email_token=AAXX2KLFPWIPZGYWE5P6CNTPWHRBVA5CNFSM4HN5UHLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVXNG6Y#issuecomment-493802363>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAXX2KK2YLM5VLRQER3CGVTPWHRBVANCNFSM4HN5UHLA> .

-- Fabien

normanmaurer · 2019-05-20T05:04:17Z

...c/test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequestTest.java

@@ -0,0 +1,82 @@
+/*
+ * Copyright 2013 The Netty Project


normanmaurer · 2019-05-20T05:05:11Z

...c/test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequestTest.java

+                            FullBinaryMemcacheRequest actual) {
+        assertNotSame(expected, actual);
+
+        assertTrue(ByteBufUtil.equals(expected.key(), actual.key()));


just use assertEquals(expected.key(), actual.key() .... same for the other lines

normanmaurer · 2019-05-20T05:06:20Z

...c/test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequestTest.java

+        assertEquals(expected.dataType(), actual.dataType());
+        assertEquals(expected.totalBodyLength(), actual.totalBodyLength());
+        assertEquals(expected.opaque(), actual.opaque());
+        assertEquals(expected.cas(), actual.cas());


call expected.release() and actual.release() in a finally block.

Getting a ref count exception when running mvn test when I do that. The buffers are unpooled.

hmm can I see the exception ?

ah I now see why... you sometimes pass the same buffer / request .

Just change it to when you pass in the same instance to use retainedDuplicate(). This will ensure you will also increment the reference count

Addressed.
I've added some code to track what refs should be released and cleaned up in a junit @After method.

See above... I would prefer if you just do it directly in the tests

normanmaurer · 2019-05-20T05:06:36Z

.../test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponseTest.java

@@ -0,0 +1,82 @@
+/*
+ * Copyright 2013 The Netty Project


normanmaurer · 2019-05-20T05:06:44Z

.../test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponseTest.java

+
+        assertTrue(ByteBufUtil.equals(expected.key(), actual.key()));
+        assertTrue(ByteBufUtil.equals(expected.extras(), actual.extras()));
+        assertTrue(ByteBufUtil.equals(expectedContent, actual.content()));


normanmaurer · 2019-05-20T05:06:50Z

.../test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheResponseTest.java

+        assertEquals(expected.totalBodyLength(), actual.totalBodyLength());
+        assertEquals(expected.opaque(), actual.opaque());
+        assertEquals(expected.cas(), actual.cas());
+    }


normanmaurer · 2019-05-20T05:07:11Z

...ache/src/main/java/io/netty/handler/codec/memcache/binary/AbstractBinaryMemcacheMessage.java

@@ -232,4 +232,15 @@ public BinaryMemcacheMessage touch(Object hint) {
        }
        return this;
    }
+
+    protected void copyMeta(AbstractBinaryMemcacheMessage dst) {


package-private ?

please add javadocs

normanmaurer · 2019-05-21T05:19:35Z

@netty-bot test this please

normanmaurer · 2019-05-21T07:27:01Z

...cache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheRequest.java

+     *
+     * @param dst The instance where to copy the metadata of this instance to
+     */
+    void copyMeta(DefaultBinaryMemcacheRequest dst) {


missing @Override ?

no the signature of the methods are different:

super: void copyMeta(AbstractBinaryMemcacheMessage dst) this : void copyMeta(DefaultBinaryMemcacheRequest dst)

normanmaurer · 2019-05-21T07:27:15Z

...ache/src/main/java/io/netty/handler/codec/memcache/binary/DefaultBinaryMemcacheResponse.java

+     *
+     * @param dst The instance where to copy the metadata of this instance to
+     */
+    void copyMeta(DefaultBinaryMemcacheResponse dst) {


missing @Override ?

no the signature of the methods are different:

super: void copyMeta(AbstractBinaryMemcacheMessage dst) this : void copyMeta(DefaultBinaryMemcacheResponse dst)

normanmaurer · 2019-05-21T07:27:52Z

...c/test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequestTest.java

+
+    @Before
+    public void setUp() {
+        refToRelease = new LinkedList<ReferenceCounted>();


honestly I think it would be a lot cleaner if you just release stuff in the tests itself.

normanmaurer · 2019-05-21T07:28:07Z

...c/test/java/io/netty/handler/codec/memcache/binary/DefaultFullBinaryMemcacheRequestTest.java

+        assertEquals(expected.dataType(), actual.dataType());
+        assertEquals(expected.totalBodyLength(), actual.totalBodyLength());
+        assertEquals(expected.opaque(), actual.opaque());
+        assertEquals(expected.cas(), actual.cas());


See above... I would prefer if you just do it directly in the tests

Motivations ----------- Calling `copy()`, `duplicate()` or `replace()` on `FullBinaryMemcacheResponse` or `FullBinaryMemcacheRequest` instances should copy status, opCode, etc. that are defined in `AbstractBinaryMemcacheMessage`. Modifications ------------- - Modified duplicate, copy and replace methods in DefaultFullBinaryMemcacheRequest and DefaultFullBinaryMemcacheResponse to always copy metadata from parent classes. - Unit tests verifying duplicate, copy and replace methods for DefaultFullBinaryMemcacheRequest and DefaultFullBinaryMemcacheResponse copy buffers and metadata as expected. Result ------ Calling copy(), duplicate() or replace() methods on DefaultFullBinaryMemcacheRequest or DefaultFullBinaryMemcacheResponse produces valid copies with all expected metadata. Fixes netty#9159

normanmaurer · 2019-05-21T15:12:20Z

@netty-bot test this please

normanmaurer · 2019-05-22T07:05:04Z

@netty-bot test this please

normanmaurer · 2019-05-22T09:06:00Z

@fabienrenaud thanks a lot!

Motivations ----------- Calling `copy()`, `duplicate()` or `replace()` on `FullBinaryMemcacheResponse` or `FullBinaryMemcacheRequest` instances should copy status, opCode, etc. that are defined in `AbstractBinaryMemcacheMessage`. Modifications ------------- - Modified duplicate, copy and replace methods in DefaultFullBinaryMemcacheRequest and DefaultFullBinaryMemcacheResponse to always copy metadata from parent classes. - Unit tests verifying duplicate, copy and replace methods for DefaultFullBinaryMemcacheRequest and DefaultFullBinaryMemcacheResponse copy buffers and metadata as expected. Result ------ Calling copy(), duplicate() or replace() methods on DefaultFullBinaryMemcacheRequest or DefaultFullBinaryMemcacheResponse produces valid copies with all expected metadata. Fixes #9159

fabienrenaud mentioned this pull request May 19, 2019

codec-memcache: copy, duplicate, replace not copying full request/response metadata #9159

Closed

fabienrenaud force-pushed the issue-9159 branch from 71c3616 to 6ed0df6 Compare May 20, 2019 01:56

normanmaurer requested changes May 20, 2019

View reviewed changes

fabienrenaud force-pushed the issue-9159 branch 3 times, most recently from ec87354 to ca2db19 Compare May 20, 2019 15:41

normanmaurer requested changes May 21, 2019

View reviewed changes

fabienrenaud force-pushed the issue-9159 branch from ca2db19 to f9aaa68 Compare May 21, 2019 12:58

normanmaurer added this to the 4.1.37.Final milestone May 22, 2019

normanmaurer merged commit 52c5389 into netty:4.1 May 22, 2019

fabienrenaud deleted the issue-9159 branch May 23, 2019 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

codec-memcache: copy metadata in binary full request response #9160

codec-memcache: copy metadata in binary full request response #9160

fabienrenaud commented May 19, 2019 •

edited

Loading

netty-bot commented May 19, 2019

fabienrenaud commented May 19, 2019 via email

normanmaurer May 20, 2019

fabienrenaud May 20, 2019

normanmaurer May 20, 2019

fabienrenaud May 20, 2019

normanmaurer May 20, 2019

fabienrenaud May 20, 2019

normanmaurer May 20, 2019

normanmaurer May 20, 2019

fabienrenaud May 20, 2019 •

edited

Loading

normanmaurer May 21, 2019

fabienrenaud May 21, 2019

normanmaurer May 20, 2019

fabienrenaud May 20, 2019

normanmaurer May 20, 2019

fabienrenaud May 20, 2019

normanmaurer May 20, 2019

fabienrenaud May 20, 2019

normanmaurer May 20, 2019

fabienrenaud May 20, 2019

normanmaurer commented May 21, 2019

normanmaurer May 21, 2019

fabienrenaud May 21, 2019

normanmaurer May 21, 2019

fabienrenaud May 21, 2019

normanmaurer May 21, 2019

fabienrenaud May 21, 2019

normanmaurer May 21, 2019

normanmaurer commented May 21, 2019

normanmaurer commented May 22, 2019

normanmaurer commented May 22, 2019

codec-memcache: copy metadata in binary full request response #9160

codec-memcache: copy metadata in binary full request response #9160

Conversation

fabienrenaud commented May 19, 2019 • edited Loading

Motivations

Modifications

Result

netty-bot commented May 19, 2019

fabienrenaud commented May 19, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabienrenaud May 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented May 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanmaurer commented May 21, 2019

normanmaurer commented May 22, 2019

normanmaurer commented May 22, 2019

fabienrenaud commented May 19, 2019 •

edited

Loading

fabienrenaud May 20, 2019 •

edited

Loading