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

ISPN-14207 Make MemcachedDecoder non blocking #10379

Merged
merged 1 commit into from Oct 15, 2022

Conversation

pruivo
Copy link
Member

@pruivo pruivo commented Oct 10, 2022

@pruivo
Copy link
Member Author

pruivo commented Oct 11, 2022

looks like I have some failures in integration tests...

@tristantarrant tristantarrant added this to the 14.0.1.Final milestone Oct 11, 2022
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.

Looks good, just one real question regarding stats blocking.

@pruivo
Copy link
Member Author

pruivo commented Oct 11, 2022

@diegolovison can we have a perf run with memcached client?

}

private void sendResponseOrdered(Channel ch, CompletionStage<Object> rsp) {
new ResponseEntry(this, ch).queueResponse(rsp);
Copy link
Member

Choose a reason for hiding this comment

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

This makes me think we should add a test that does 2 operations in parallel with the non blocking mecached API. Or do we already have that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I can remove the get() from here: https://github.com/infinispan/infinispan/pull/10379/files#diff-49239ed285f72772402428f971aa797f4a8be001e4cf22c5a31765afe0e207ebR50

The test only needs the last get() to make sure the flush() is not executed concurrently with the puts.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did the change above ^

Copy link
Member

Choose a reason for hiding this comment

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

Only problem is that we aren't checking the response and they aren't interesting in the test. I would say we should do this to a test that does two gets in parallel that get different values in the response, just to make sure the responses aren't interleaved.

Copy link
Member Author

Choose a reason for hiding this comment

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

added org.infinispan.server.functional.MemcachedOperations#testConcurrentGets

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks 👍

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, waiting on CI/performance checks

@diegolovison
Copy link
Contributor

@pruivo
Copy link
Member Author

pruivo commented Oct 14, 2022

@diegolovison are you sure the job did run the right thing?

  • the snapshot commit is from 8 days ago.
  • the logs show the test run for 240 sec but the report shows almost a ~400 sec duration.
  • the radargun version used is different

@diegolovison
Copy link
Contributor

http://perfrepo.mw.lab.eng.bos.redhat.com/reports/tableComparisonReport/19586

@tristantarrant tristantarrant merged commit 30f69fa into infinispan:main Oct 15, 2022
@tristantarrant
Copy link
Member

Merged, thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants