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

librados: Add rados_aio_exec to the C API #11709

Merged
merged 3 commits into from Nov 18, 2016
Merged

librados: Add rados_aio_exec to the C API #11709

merged 3 commits into from Nov 18, 2016

Conversation

iain-buclaw-sociomantic
Copy link
Contributor

This should make adding rados_aio_exec to the C API possible.

Signed-off-by: Iain Buclaw iain.buclaw@sociomantic.com

@iain-buclaw-sociomantic
Copy link
Contributor Author

I also have an implementation of rados_aio_exec, tests for C and C++ versions, rados python bindings, and tests for the python ready to commit also.

However I first want to make sure that having a separate override is needed in order to call aio_exec from C. Maybe there is a better way to do it, but it looks like C_aio_Ack::finish needs out_buf set to copy the data back.

@iain-buclaw-sociomantic iain-buclaw-sociomantic changed the title librados: Add aio_exec with char pointer buffer librados: Add aio_exec override with char pointer buffer Nov 1, 2016
@liewegas liewegas added this to the kraken milestone Nov 1, 2016
@liewegas
Copy link
Member

liewegas commented Nov 3, 2016

This looks right! Can you add a test to test/librados/aio.cc?

@iain-buclaw-sociomantic
Copy link
Contributor Author

@liewegas - Sure, should I proceed in adding all other commits I've got lined up also?

@liewegas
Copy link
Member

liewegas commented Nov 4, 2016

Only if they're also aio librados additions; otherwise better to put them in a separate PR.

@iain-buclaw-sociomantic
Copy link
Contributor Author

Yes they are, though I'd probably put the python bindings in a separate PR.

@iain-buclaw-sociomantic iain-buclaw-sociomantic changed the title librados: Add aio_exec override with char pointer buffer librados: Add rados_aio_exec to the C API Nov 4, 2016
@iain-buclaw-sociomantic
Copy link
Contributor Author

Added rados_aio_exec function, tests using the hello plugin, and rebased against master.

ASSERT_NE(my_completion2, my_completion_null);
bufferlist in, out;
ASSERT_EQ(0, test_data.m_ioctx.aio_exec("foo", my_completion2,
"hello", "say_hello", in, &out);
Copy link
Member

Choose a reason for hiding this comment

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

missing close parens

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed there is, I thought jenkins ran the testsuite.

@iain-buclaw-sociomantic
Copy link
Contributor Author

Added missing closing parenthesis in both aio_exec tests, and rebased the branch.

This should make adding rados_aio_exec to the C API possible.

Signed-off-by: Iain Buclaw <iain.buclaw@sociomantic.com>
Makes available in the C API what is already exposed in the libradospp.

Signed-off-by: Iain Buclaw <iain.buclaw@sociomantic.com>
rados_completion_t my_completion2;
ASSERT_EQ(0, rados_aio_create_completion((void*)&test_data,
set_completion_completeEC, set_completion_safeEC, &my_completion2));
char out[128]
Copy link
Contributor

Choose a reason for hiding this comment

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

add a ";" please.

@iain-buclaw-sociomantic could you try to compile the test and run it locally? you can

make ceph_test_rados_api_aio
ctest -R ceph_test_rados_api_aio // with a running vstart cluster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks @tchaikov

The C test checks the functionality of the new IoCtxImpl::aio_exec
override that accepts a char* buffer. The C++ test does the same action,
but internally uses the other override that accepts a bufferlist*.

Signed-off-by: Iain Buclaw <iain.buclaw@sociomantic.com>
@iain-buclaw-sociomantic
Copy link
Contributor Author

@tchaikov - Verified that tests pass locally, rebased the branch.

@liewegas
Copy link
Member

passed a rados run, lgtm

@ghost
Copy link

ghost commented Nov 17, 2016

jenkins test this please (flake8 error fixed in master)

@liewegas liewegas dismissed tchaikov’s stale review November 18, 2016 00:49

compile error was fixed

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