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

compressor: fixes and tests; disable zlib isal (it's broken) #11349

Merged
merged 9 commits into from Oct 7, 2016

Conversation

liewegas
Copy link
Member

@liewegas liewegas commented Oct 6, 2016

No description provided.

@liewegas
Copy link
Member Author

liewegas commented Oct 6, 2016

@Ved-vampir the zlib ISA-L code seems to be broken. I've disabled it in this PR. Can you take a look? Just revert the commit and run the unittest_compressor to see it fail.

@ifed01
Copy link
Contributor

ifed01 commented Oct 6, 2016

@liewegas She's at maternity leave at the moment, I'll take a look..

@liewegas
Copy link
Member Author

liewegas commented Oct 6, 2016 via email

@liewegas
Copy link
Member Author

liewegas commented Oct 6, 2016

@ifed01 ok to merge this in the meantime?

@ifed01
Copy link
Contributor

ifed01 commented Oct 6, 2016

Will review in a couple hours...

@ifed01
Copy link
Contributor

ifed01 commented Oct 6, 2016

@liewegas see PR #11351 for statfs fix

@@ -0,0 +1,153 @@
// -*- mode:C++; tab-width:8; c-basic-offset:2; indent-tabs-mode:t -*-
Copy link
Contributor

Choose a reason for hiding this comment

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

Test cases you removed were Intended for plugin load verification only. There is a couple more named test_compression_snappy (_zlib) that verify compression/decompression functionality. IMHO worth combining into a single file too.

@liewegas
Copy link
Member Author

liewegas commented Oct 6, 2016

yeah, i was about to do that but the zlib one has isa-related tests so i
deferred for now. i'll look again

g_conf->set_val("compressor_zlib_isal", "true");
g_ceph_context->_conf->apply_changes(NULL);
} else if (isal == "noisal") {
g_conf->set_val("compressor_zlib_isal", "false");
Copy link
Contributor

Choose a reason for hiding this comment

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

Undo to default on teardown?

@liewegas
Copy link
Member Author

liewegas commented Oct 6, 2016 via email

This obsoletes the per-plugin plugin tests, which are tedious anyway.

Signed-off-by: Sage Weil <sage@redhat.com>
4 bytes is enough for small buffers, but fails for larger buffers because
snappy encodes the length as a varint.  8 is enough.

Signed-off-by: Sage Weil <sage@redhat.com>
We dynamically enable this if the necessary processor features are present.
Allow this probing to be disabled explicitly.

Signed-off-by: Sage Weil <sage@redhat.com>
This fails the unit tests.  Do not reenable until it is fixed!

Signed-off-by: Sage Weil <sage@redhat.com>
Just so it runs a bit faster!

Signed-off-by: Sage Weil <sage@redhat.com>
This includes zlib and isal interop.

Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
@@ -48,6 +49,13 @@ class CompressionTest : public ::testing::Test,
}
}
cout << "[plugin " << plugin << " (" << GetParam() << ")]" << std::endl;

// note for later
old_zlib_isal = g_conf->compressor_zlib_isal;
Copy link
Contributor

Choose a reason for hiding this comment

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

Move upward prior to applying new values?

@ifed01
Copy link
Contributor

ifed01 commented Oct 7, 2016

LGTM except old_zlib_isal setup.

Signed-off-by: Sage Weil <sage@redhat.com>
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas liewegas merged commit df4973c into ceph:master Oct 7, 2016
@liewegas liewegas deleted the wip-compressor branch October 7, 2016 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants