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.

Sign up for GitHub

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

Jump to bottom

ISPN-8726 Memcached connector rewrite #10655

Merged
merged 3 commits into from May 11, 2023
Merged

ISPN-8726 Memcached connector rewrite #10655

merged 3 commits into from May 11, 2023

Conversation

tristantarrant
Copy link
Member

@tristantarrant tristantarrant force-pushed the ISPN-8726/memcached_rewrite branch 6 times, most recently from bb20468 to c831a76 Compare February 27, 2023 17:51
@tristantarrant tristantarrant force-pushed the ISPN-8726/memcached_rewrite branch 2 times, most recently from b47230a to e54610b Compare March 3, 2023 14:24
wburns
wburns reviewed Mar 3, 2023
View reviewed changes
Copy link
Member

@wburns wburns left a comment

Choose a reason for hiding this comment

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

Review for the authentication and single port commit

server/core/src/main/java/org/infinispan/server/core/MagicByteDetector.java Outdated Show resolved Hide resolved
server/resp/src/main/java/org/infinispan/server/resp/RespDetector.java Show resolved Hide resolved
server/resp/src/main/java/org/infinispan/server/resp/logging/Log.java Show resolved Hide resolved
@tristantarrant tristantarrant force-pushed the ISPN-8726/memcached_rewrite branch 3 times, most recently from b8fd3b1 to acf1d3a Compare March 7, 2023 12:25
wburns
wburns requested changes Mar 8, 2023
View reviewed changes
Copy link
Member

@wburns wburns left a comment •
edited

Choose a reason for hiding this comment

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

I got through a part of the files. Will need to look at the rest tomorrow.

server/memcached/src/main/java/org/infinispan/server/memcached/MemcachedAutoDetector.java Outdated Show resolved Hide resolved
server/memcached/src/main/java/org/infinispan/server/memcached/binary/BinaryResponse.java Outdated Show resolved Hide resolved
server/memcached/src/main/java/org/infinispan/server/memcached/binary/BinaryResponse.java Outdated Show resolved Hide resolved
server/memcached/src/main/java/org/infinispan/server/memcached/binary/BinaryResponse.java Outdated Show resolved Hide resolved
server/memcached/src/main/java/org/infinispan/server/memcached/binary/BinaryHeader.java Outdated Show resolved Hide resolved
server/memcached/src/main/java/org/infinispan/server/memcached/binary/BinaryCommand.java Outdated Show resolved Hide resolved
server/memcached/src/main/java/org/infinispan/server/memcached/MemcachedTextDetector.java Show resolved Hide resolved
server/memcached/src/main/java/org/infinispan/server/memcached/MemcachedStats.java Outdated Show resolved Hide resolved
server/memcached/src/main/java/org/infinispan/server/memcached/MemcachedBaseDecoder.java Outdated Show resolved Hide resolved
server/memcached/src/main/java/org/infinispan/server/memcached/binary/BinaryOpDecoder.java Outdated Show resolved Hide resolved
@tristantarrant tristantarrant force-pushed the ISPN-8726/memcached_rewrite branch 2 times, most recently from a17d23a to d3204f4 Compare March 9, 2023 09:15
wburns
wburns requested changes Mar 10, 2023
View reviewed changes
Copy link
Member

@wburns wburns left a comment

Choose a reason for hiding this comment

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

I did not look at every file, but I think I got the important ones. Many are just minor refactorings. Nothing really problematic other than the small bug in the Intrinsics. Most things are suggestions for optimizations.

core/src/test/java/org/infinispan/encoding/impl/EncoderWrappingTest.java Outdated Show resolved Hide resolved
core/src/test/java/org/infinispan/encoding/impl/EncoderWrappingTest.java Outdated Show resolved Hide resolved
core/src/test/java/org/infinispan/encoding/impl/EncoderWrappingTest.java Outdated Show resolved Hide resolved
maven-settings.xml Outdated Show resolved Hide resolved
server/core/src/main/java/org/infinispan/server/core/MagicByteDetector.java Outdated Show resolved Hide resolved
...ached/src/test/java/org/infinispan/server/memcached/test/MemcachedBlockHoundIntegration.java Outdated
@@ -13,6 +13,6 @@ public class MemcachedBlockHoundIntegration implements BlockHoundIntegration {

@Override
public void applyTo(BlockHound.Builder builder) {
builder.allowBlockingCallsInside(MemcachedTestingUtil.class.getName() + "$2", "getDecoder");
//builder.allowBlockingCallsInside(MemcachedTestingUtil.class.getName() + "$2", "getDecoder");
Copy link
Member

Choose a reason for hiding this comment

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

Just go ahead and delete the line

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

server/memcached/src/test/java/org/infinispan/server/memcached/test/DefaultLogger.java Outdated
*/
@Override
public boolean isTraceEnabled() {
return (true);
Copy link
Member

Choose a reason for hiding this comment

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

Why the parenthesis?

Copy link
Member Author

Choose a reason for hiding this comment

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

Weird! Done

server/memcached/src/main/java/org/infinispan/server/memcached/text/TextAuthDecoder.java
ByteBuf buf = channel.alloc().ioBuffer();
buf.writeBytes(STORED);
channel.writeAndFlush(buf);
channel.pipeline().replace("decoder", "decoder", new TextOpDecoderImpl(server, subject));
Copy link
Member

Choose a reason for hiding this comment

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

Are we 100% sure the client may not send another command on the socket before receiving the STORED response? I worry because it would be possible to process those commands before the new decoder is put in place causing other issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

The authentication section in the protocol doc doesn't really say much. How would we protected against such an eventuality ?

Copy link
Member

Choose a reason for hiding this comment

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

The only way I know is not to process the next command until the previous one is completed like we are doing in RESP. But this could be a non issue, I was just curious if you knew.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking a little more about this: authentication is a synchronous thing. Let's leave it like that and in case a client behaves weirdly we can go back on it

Copy link
Member

Choose a reason for hiding this comment

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

Sure, np.

server/memcached/src/main/java/org/infinispan/server/memcached/binary/BinaryAuthDecoder.java Show resolved Hide resolved
server/memcached/src/main/java/org/infinispan/server/memcached/MemcachedStats.java Outdated
* @since 15.0
**/
public class MemcachedStats {
public static final AtomicLongFieldUpdater INCR_MISSES = newUpdater(MemcachedStats.class, "incrMisses");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static final AtomicLongFieldUpdater INCR_MISSES = newUpdater(MemcachedStats.class, "incrMisses");
public static final AtomicLongFieldUpdater<MemcachedStats> INCR_MISSES = newUpdater(MemcachedStats.class, "incrMisses");

and the following fields to remove some warnings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@tristantarrant
Copy link
Member Author

@wburns repushed

wburns
wburns approved these changes May 3, 2023
View reviewed changes
Copy link
Member

@wburns wburns left a comment

Choose a reason for hiding this comment

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

LGTM, needs rebase though

@wburns
Copy link
Member

wburns commented May 4, 2023 •
edited

Looks like there are some related test failures https://ci.infinispan.org/job/Infinispan/job/PR-10655/20/, especially the leaked threads

@tristantarrant tristantarrant force-pushed the ISPN-8726/memcached_rewrite branch 2 times, most recently from 641a267 to 6f4712f Compare May 10, 2023 06:19
@wburns
Copy link
Member

wburns commented May 10, 2023

Authentication tests started failing now :(

@tristantarrant
Copy link
Member Author

Authentication tests started failing now :(

Yes, it's an issue with the memcached client. Investigating

@tristantarrant
ISPN-8726 Memcached connector rewrite
949a903 
* use our protocol parser generator
* implement both the text and binary protocol
* implement SASL authentication
* implement text authentication
* implement automatic protocol detection
* enable memcached by default

ISPN-13548 Allow cache expiration for Memcached
@tristantarrant
Copy link
Member Author

CI looks much much better now :)

@wburns wburns merged commit 59593d6 into infinispan:main May 11, 2023
3 of 4 checks passed
@wburns
Copy link
Member

wburns commented May 11, 2023

Integrated into main, thanks @tristantarrant !

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

@wburns wburns wburns approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@tristantarrant @wburns @ryanemerson