osd,compressor: Expose compression algorithms via MOSDBoot. #20558

chardan · 2018-02-23T10:13:14Z

Modifies Compressor to expose a set of strings (already hardcoded)
that are accessible elsewhere.

Adds metadata to MOSDBoost containing a list of compression algorithms
made available via the plugin interface.

http://tracker.ceph.com/issues/22420

Signed-off-by: Jesse Williamson jwilliamson@suse.de

gregsfortytwo · 2018-02-24T01:38:51Z

src/osd/OSD.cc

@@ -12,14 +12,19 @@
 * Foundation.  See file COPYING.
 *
 */
-#include "acconfig.h"


Is there a reason you reordered these include orders? There are some annoyingly delicate dependencies and the cross-platform stuff won't get picked up in our own testing.

Hm, I didn't know about the cross-platform issue! I have to say it's almost become reflexive. Sage has asked before that headers be tidied up "while we're in there", but perhaps acconfig wasn't on the menu. I've moved it back to the top. I can also revert all the re-ordering if that's the best thing to do.

Well, I'm not saying there definitely is a problem; like I said the issues are fragile. And while our config header is a good guess for which one matters, it'll really be anything that inherits any kind of definition where we override or conflict with the standard ones. (We see the same thing with our own definition of assert, for instance, although that one's markedly less fragile.)
But in general I get queasy around seeing headers rearranged; the cosmetics aren't worth it in a system with as cross-unit dependencies and invariants as Ceph has.

gregsfortytwo · 2018-02-24T01:42:43Z

src/osd/OSD.cc

+  string s(os.str());
+
+  // No ostream_joiner yet, so we'll need to do some trimming:
+  if(s[s.size()] == ',')


This should probably be size() - 1?

Good catch! "Someone" didn't have enough coffee! Thanks.

gregsfortytwo · 2018-02-26T22:36:40Z

jenkins test make check

gregsfortytwo · 2018-02-26T22:55:10Z

@chardan, these are real build errors. Looks like _collect_compression_algorithms() doesn't match the declaration and implementation params

chardan · 2018-02-28T12:11:58Z

@gregsfortytwo Should be addressed now, thank you!

gregsfortytwo · 2018-02-28T21:48:33Z

src/compressor/Compressor.cc


-  return boost::optional<CompressionAlgorithm>();
+  for (int pos = 0; pos != sizeof(compression_algorithm_names); ++pos) {


Why != instead of <= in this loop? I thought that was generally considered bad because if the loop mutates and you accidentally skip over the end condition, you're in trouble...

@gregsfortytwo Should be < instead of <=

If a static array mutates, you're in real trouble! ;-)

gregsfortytwo · 2018-02-28T21:49:08Z

Reviewed-by: Greg Farnum gfarnum@redhat.com

tchaikov · 2018-03-01T03:31:46Z

src/compressor/Compressor.cc


-  return boost::optional<CompressionAlgorithm>();
+  for (int pos = 0; pos != sizeof(compression_algorithm_names); ++pos) {
+	if(compression_algorithm_names[pos] == s)


add a space after if, also you could put

auto found = find(begin(compression_algorithm_names), end(compression_algorithm_names), s); if (found == end(compression_algorithm_names)) { return {}; } else { return found - begin(compression_algorithm_names); }

tchaikov · 2018-03-01T03:32:18Z

src/compressor/Compressor.cc

-  default: return "???";
-  }
+
+  if(a < 0 || a >= COMP_ALG_LAST) 


add a space after if.

tchaikov · 2018-03-01T03:33:49Z

src/compressor/Compressor.h

+
+  // The names in this array must match the above values:
+  constexpr static char const* const compression_algorithm_names[] = {
+	"none",


please note our tab-width is 8. and the indent offset is 2. so this indent does not follow the style guide.

It matches all the code around it...

tchaikov · 2018-03-01T03:36:06Z

src/osd/OSD.cc

+  const auto& compression_algorithm_names = Compressor::compression_algorithm_names;
+  const auto& plugin_registry = cct->get_plugin_registry()->plugins;
+
+  if(0 == plugin_registry.size())


please use empty() instead, if what you are interested is the emptiness instead of of the actual size.

tchaikov · 2018-03-01T03:38:20Z

src/osd/OSD.cc

@@ -5536,6 +5541,28 @@ void OSD::_send_boot()
  set_state(STATE_BOOTING);
 }

+std::string OSD::_collect_compression_algorithms()
+{
+  const auto& compression_algorithm_names = Compressor::compression_algorithm_names;


nit, could use a shorter name.

tchaikov · 2018-03-01T03:38:58Z

src/osd/OSD.cc

+
+  string s(os.str());
+
+  s.erase(s.find_last_of(','));


could just use

std::copy(begin(names), end(names), std::experimental::make_ostream_joiner(os, ", "));

Agreed, are we feeling ok with things in std::experimental in general, though?

we used to use facilities in the TS namespace before they are included by the std namespace. i think as long as it's part of standard, there is no reason not using it.

tchaikov · 2018-03-01T03:39:13Z

src/osd/OSD.cc

@@ -5544,6 +5571,7 @@ void OSD::_collect_metadata(map<string,string> *pm)
    // not applicable for bluestore
    (*pm)["osd_journal"] = journal_path;
  }
+


please avoid adding empty lines.

I think it vastly improves readability.

see https://github.com/ceph/ceph/blob/master/CodingStyle and https://google.github.io/styleguide/cppguide.html#Vertical_Whitespace. also it'd be better not changing the area your are not touching.

tchaikov · 2018-03-01T03:39:45Z

src/osd/OSD.h

@@ -1927,6 +1927,7 @@ class OSD : public Dispatcher,
  void _preboot(epoch_t oldest, epoch_t newest);
  void _send_boot();
  void _collect_metadata(map<string,string> *pmeta);
+	std::string _collect_compression_algorithms();


wrong indent.

It's to show that this function depends on/is only used by the other function.

i'd suggest follow our coding style before we enforce yours.

tchaikov · 2018-03-01T03:41:05Z

~~dropped needs-qa due to #20558 (comment)~~

tchaikov · 2018-03-02T15:50:29Z

src/compressor/Compressor.cc

+  auto p = std::find_if(std::cbegin(compression_algorithms), std::cend(compression_algorithms),
+		   [a](const auto& kv) { return kv.second == a; });
+
+  if(std::cend(compression_algorithms) == p)


again, please add a space after if.

tchaikov

aside from couple of formatting issues, lgtm.

tchaikov · 2018-03-10T01:23:28Z

could you prefix the title of your commit message with the subcomponent your are changing ? see https://github.com/ceph/ceph/blob/master/SubmittingPatches.rst#3-describe-your-changes

tchaikov · 2018-03-10T01:23:46Z

retest this please.

yuriw · 2018-03-13T15:07:23Z

retest this please.

chardan · 2018-03-13T18:12:58Z

@yuriw Real errors, sorry-- working on them, I'm juggling a bunch of stuff right now! I'll update soon.

Modifies Compressor to expose a set of strings (already hardcoded) that are accessible elsewhere. Adds metadata to MOSDBoost containing a list of compression algorithms made available via the plugin interface. Fixes: http://tracker.ceph.com/issues/22420 Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

chardan · 2018-03-16T19:19:20Z

@yuriw Thanks for your patience!

tchaikov · 2018-03-27T14:42:27Z

tchaikov · 2018-03-29T00:56:57Z

#21106 adds the zstd compressor back. it was removed in this change.

jcsp · 2018-05-03T13:13:03Z

@chardan it looks like this patch is not working as intended, because the plugin registry is only populated at the point a plugin is loaded for use, so we're getting empty lists in this metadata field in practice.

This needs a follow up change to enable PluginRegistry to scan its plugin dir at init time (probably what the unimplemented preload function in the original header would have done), and/or perhaps we should revert this change until we have that, to avoid confusing people with a blank field?

cbodley · 2018-05-03T13:48:25Z

since compression_algorithms is the list of types enabled at build time, why not report those verbatim?

the plugin registry check just confirms that the shared library is present/loadable - but if we have control over packaging/installation, can't we just assume they are?

the manager itself has access to compression_algorithms, so it wouldn't even need to ask the osds for it

tchaikov · 2018-05-03T14:00:42Z

agreed, and probably it's more practical / useful to have a single "compression_algorithms" report on monitor instead from osd.

as in a large scale cluster, it's tedious to check every OSD nodes and calculate the intersection of the compression_algorithm of them, and then decide which compression(s) is available.

cbodley

sorry for belated review

cbodley · 2018-05-03T16:37:29Z

src/compressor/Compressor.h

+    COMP_ALG_LAST   //the last value for range checks
+  };
+
+  inline static const std::map<const std::string, const CompressionAlgorithm> compression_algorithms {


i opened #21801 to make this a constexpr initializer_list

this exposes compression_algorithms directly as an interface to determine which are enabled. consider extending the static functions to do this instead. for example:

static constexpr std::optional<const char*> get_comp_alg_name(int a); static constexpr std::optional<CompressionAlgorithm> get_comp_alg_type(std::string_view s);

the caller can loop over [0,COMP_ALG_LAST) and call get_comp_alg_name(i) to include the names of each algorithm enabled. these functions can be constexpr thanks to initializer_list and the constexpr constructors of std::optional and std::string_view. std::find_if() isn't constexpr until c++20, but a ranged-for loop would work instead

(this would also address the comment return "???"; // It would be nice to revise this...)

Yeah, 'return "???"' was what the prior code did, and frankly I'm not a fan of that. I decided to lift up only a single level of abstraction because I didn't know how much other code might be using it, but there's certainly no reason it can't be revisited! Thanks for your thoughtful comments.

cbodley · 2018-05-03T16:51:31Z

src/osd/OSD.cc

+  ostringstream os;
+
+  copy_if(begin(compression_algorithms), end(compression_algorithms),
+          make_ostream_joiner(os, ", "),


it looks like this will print pairs instead of just the names

chardan self-assigned this Feb 23, 2018

chardan requested a review from jcsp February 23, 2018 10:13

chardan force-pushed the jfw-wip-show_compression_methods-22420 branch 8 times, most recently from 042eb2b to 8515dd5 Compare February 23, 2018 15:54

batrick added core needs-review labels Feb 23, 2018

gregsfortytwo requested changes Feb 24, 2018

View reviewed changes

chardan force-pushed the jfw-wip-show_compression_methods-22420 branch 2 times, most recently from 6fa22e5 to f5d102e Compare February 24, 2018 10:49

chardan force-pushed the jfw-wip-show_compression_methods-22420 branch from f5d102e to 544429e Compare February 27, 2018 10:36

gregsfortytwo reviewed Feb 28, 2018

View reviewed changes

gregsfortytwo approved these changes Feb 28, 2018

View reviewed changes

gregsfortytwo added needs-qa and removed needs-review labels Feb 28, 2018

tchaikov removed the needs-qa label Mar 1, 2018

tchaikov reviewed Mar 1, 2018

View reviewed changes

src/compressor/Compressor.cc Outdated

default: return "???";

}

if(a < 0 || a >= COMP_ALG_LAST)

Copy link

Contributor

tchaikov Mar 1, 2018

Choose a reason for hiding this comment

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

add a space after if.

tchaikov reviewed Mar 1, 2018

View reviewed changes

tchaikov requested changes Mar 1, 2018

View reviewed changes

tchaikov reviewed Mar 2, 2018

View reviewed changes

chardan force-pushed the jfw-wip-show_compression_methods-22420 branch 6 times, most recently from 1d76263 to 2d3842f Compare March 9, 2018 09:39

tchaikov approved these changes Mar 10, 2018

View reviewed changes

chardan force-pushed the jfw-wip-show_compression_methods-22420 branch from 2d3842f to d0c5c2e Compare March 12, 2018 20:14

chardan changed the title ~~Expose compression algorithms via MOSDBoot. Implements 22420.~~ osd,compressor: Expose compression algorithms via MOSDBoot. Mar 12, 2018

chardan force-pushed the jfw-wip-show_compression_methods-22420 branch from d0c5c2e to 52243cf Compare March 12, 2018 22:04

tchaikov removed the needs-qa label Mar 14, 2018

chardan force-pushed the jfw-wip-show_compression_methods-22420 branch from 52243cf to 16c3552 Compare March 15, 2018 00:26

tchaikov added the needs-qa label Mar 16, 2018

tchaikov added the wip-kefu-testing label Mar 26, 2018

tchaikov merged commit a1f204e into ceph:master Mar 27, 2018

cbodley reviewed May 3, 2018

View reviewed changes

jcsp mentioned this pull request May 4, 2018

mon,osd: dump "compression_algorithms" in "mon metadata" #21809

Merged


		return boost::optional<CompressionAlgorithm>();
		for (int pos = 0; pos != sizeof(compression_algorithm_names); ++pos) {

osd,compressor: Expose compression algorithms via MOSDBoot. #20558

osd,compressor: Expose compression algorithms via MOSDBoot. #20558

Conversation

chardan commented Feb 23, 2018

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

gregsfortytwo commented Feb 26, 2018

gregsfortytwo commented Feb 26, 2018

chardan commented Feb 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregsfortytwo commented Feb 28, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chardan Mar 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tchaikov commented Mar 1, 2018 • edited

Choose a reason for hiding this comment

tchaikov left a comment

Choose a reason for hiding this comment

tchaikov commented Mar 10, 2018

tchaikov commented Mar 10, 2018

yuriw commented Mar 13, 2018

chardan commented Mar 13, 2018

chardan commented Mar 16, 2018

tchaikov commented Mar 27, 2018

tchaikov commented Mar 29, 2018

jcsp commented May 3, 2018

cbodley commented May 3, 2018

tchaikov commented May 3, 2018

cbodley left a 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

chardan Mar 1, 2018 •

edited

tchaikov commented Mar 1, 2018 •

edited