[PATCH] sharded: support for non-cooperative service types

[Avi Kivity]

The sharded<Service> template was designed with cooperating service classes
in mind. This cooperation is in the form of providing a stop() method for
orderly shutdown.

The sharded<> template is also useful for non-cooperating services: for example,
std::unordered_map<std::string can be broadcast to all shards with

sharded<std::unordered_map<sstring, sstring>> my_map;
my_map.start(initializer).get();
...
my_map.invoke_on_all([] (my_map_type& mm) { mm["x"] = "y"; }).get();
...
my_map.stop().get();

All is required is that the "Service" doesn't perform asynchronous operations (as
is usual for plain data types) or that the user stops them in some other way. But
this currently doesn't compile because the Service::stop() method is required.

To allow for with, we provide a sharded_adapter<> template. If the user wraps their
service with sharded_adapter, then stop() is no longer needed and you can distribute
any type.
---

Note: other ways to expose this are possible
- detect whether Service::stop() exists and only call it if it does
- make traits a non-template-parameter, and require the user to specialize it instead
- not do this at all, and require the user to inherit if needed and to implement a stop
method
- use a free function to stop, with a template implementation that forwards to
Service::stop()

include/seastar/core/sharded.hh | 131 ++++++++++++++++++++------------
1 file changed, 81 insertions(+), 50 deletions(-)

diff --git a/include/seastar/core/sharded.hh b/include/seastar/core/sharded.hh
index 7712105d6d..d245ba79d2 100644
--- a/include/seastar/core/sharded.hh
+++ b/include/seastar/core/sharded.hh
@@ -42,10 +42,16 @@ namespace seastar {

/// \addtogroup smp-module
/// @{

template <typename T>
+class default_sharded_traits;
+
+template <typename T>
+class plain_sharded_traits;
+
+template <typename T, typename Traits = default_sharded_traits<T>>
class sharded;

/// If sharded service inherits from this class sharded::stop() will wait
/// until all references to a service on each shard will disappear before
/// returning. It is still service's own responsibility to track its references
@@ -58,32 +64,32 @@ class async_sharded_service : public enable_shared_from_this<T> {
virtual ~async_sharded_service() {
if (_delete_cb) {
_delete_cb();
}
}
- template <typename Service> friend class sharded;
+ template <typename Service, typename Traits> friend class sharded;
};


/// \brief Provide a sharded service with access to its peers
///
/// If a service class inherits from this, it will gain a \code container()
/// \endcode method that provides access to the \ref sharded object, with which
/// it can call its peers.
-template <typename Service>
+template <typename Service, typename Traits = default_sharded_traits<Service>>
class peering_sharded_service {
sharded<Service>* _container = nullptr;
private:
- template <typename T> friend class sharded;
+ template <typename T, typename Traits1> friend class sharded;
void set_container(sharded<Service>* container) { _container = container; }
public:
peering_sharded_service() = default;
- peering_sharded_service(peering_sharded_service<Service>&&) = default;
- peering_sharded_service(const peering_sharded_service<Service>&) = delete;
- peering_sharded_service& operator=(const peering_sharded_service<Service>&) = delete;
- sharded<Service>& container() { return *_container; }
- const sharded<Service>& container() const { return *_container; }
+ peering_sharded_service(peering_sharded_service<Service, Traits>&&) = default;
+ peering_sharded_service(const peering_sharded_service<Service, Traits>&) = delete;
+ peering_sharded_service& operator=(const peering_sharded_service<Service, Traits>&) = delete;
+ sharded<Service, Traits>& container() { return *_container; }
+ const sharded<Service, Traits>& container() const { return *_container; }
};


/// Exception thrown when a \ref sharded object does not exist
class no_sharded_instance_exception : public std::exception {
@@ -91,20 +97,45 @@ class no_sharded_instance_exception : public std::exception {
virtual const char* what() const noexcept override {
return "sharded instance does not exist";
}
};

+/// Default traits definition for \ref sharded
+///
+/// This traits class is for classes that provide a Service::stop() method for
+/// orderly shutdown; typically Service::stop() signals and ongoing operation
+/// that a shutdown is in progress, and waits until they acknowledge.
+template <typename Service>
+struct default_sharded_traits {
+ static future<> stop(Service* s) { return s->stop(); }
+};
+
+/// Alternative traits definition for \ref sharded, for plain data types
+///
+/// This traits class is for types that do not provide a Service::stop() method for
+/// orderly shutdown; typically these types do not support asynchronous operation,
+/// or waiting for those asynchronous operations is done by other means.
+template <typename Service>
+struct plain_sharded_traits {
+ static future<> stop(Service* s) { return make_ready_future<>(); }
+};
+
/// Template helper to distribute a service across all logical cores.
///
/// The \c sharded template manages a sharded service, by creating
/// a copy of the service on each logical core, providing mechanisms to communicate
/// with each shard's copy, and a way to stop the service.
///
/// \tparam Service a class to be instantiated on each core. Must expose
/// a \c stop() method that returns a \c future<>, to be called when
/// the service is stopped.
-template <typename Service>
+///
+/// \tparam Traits optional adjustments to behavior; See \ref default_sharded
+/// traits for the default (for service classes that are designed to
+/// work with \c sharded) and \ref plain_sharded_traits (for classes
+/// and types that aren't designed to work with \c sharded).
+template <typename Service, typename Traits>
class sharded {
struct entry {
shared_ptr<Service> service;
promise<> freed;
};
@@ -117,17 +148,17 @@ class sharded {
}
template <typename U, bool async>
friend struct shared_ptr_make_helper;

template <typename T>
- std::enable_if_t<std::is_base_of<peering_sharded_service<T>, T>::value>
+ std::enable_if_t<std::is_base_of<peering_sharded_service<T, Traits>, T>::value>
set_container(T& service) {
service.set_container(this);
}

template <typename T>
- std::enable_if_t<!std::is_base_of<peering_sharded_service<T>, T>::value>
+ std::enable_if_t<!std::is_base_of<peering_sharded_service<T, Traits>, T>::value>
set_container(T& service) {
}
public:
/// Constructs an empty \c sharded object. No instances of the service are
/// created.
@@ -454,11 +485,11 @@ class sharded {
// do not wait for instance to be deleted since it is not going to notify us
service_deleted();
}

void track_deletion(shared_ptr<Service>& s, std::true_type) {
- s->_delete_cb = std::bind(std::mem_fn(&sharded<Service>::service_deleted), this);
+ s->_delete_cb = std::bind(std::mem_fn(&sharded::service_deleted), this);
}

template <typename... Args>
shared_ptr<Service> create_local_service(Args&&... args) {
auto s = ::seastar::make_shared<Service>(std::forward<Args>(args)...);
@@ -476,43 +507,43 @@ class sharded {
}
};

/// @}

-template <typename Service>
-sharded<Service>::~sharded() {
+template <typename Service, typename Traits>
+sharded<Service, Traits>::~sharded() {
assert(_instances.empty());
}

namespace internal {

-template <typename Service>
+template <typename Service, typename Traits>
class either_sharded_or_local {
- sharded<Service>& _sharded;
+ sharded<Service, Traits>& _sharded;
public:
- either_sharded_or_local(sharded<Service>& s) : _sharded(s) {}
- operator sharded<Service>& () { return _sharded; }
+ either_sharded_or_local(sharded<Service, Traits>& s) : _sharded(s) {}
+ operator sharded<Service, Traits>& () { return _sharded; }
operator Service& () { return _sharded.local(); }
};

template <typename T>
inline
T&&
unwrap_sharded_arg(T&& arg) {
return std::forward<T>(arg);
}

-template <typename Service>
-either_sharded_or_local<Service>
-unwrap_sharded_arg(std::reference_wrapper<sharded<Service>> arg) {
- return either_sharded_or_local<Service>(arg);
+template <typename Service, typename Traits>
+either_sharded_or_local<Service, Traits>
+unwrap_sharded_arg(std::reference_wrapper<sharded<Service, Traits>> arg) {
+ return either_sharded_or_local<Service, Traits>(arg);
}

}

-template <typename Service>
-sharded<Service>::sharded(sharded&& x) noexcept : _instances(std::move(x._instances)) {
+template <typename Service, typename Traits>
+sharded<Service, Traits>::sharded(sharded&& x) noexcept : _instances(std::move(x._instances)) {
for (auto&& e : _instances) {
set_container(e);
}
}

@@ -522,14 +553,14 @@ using on_each_shard_func = std::function<future<> (unsigned shard)>;

future<> sharded_parallel_for_each(unsigned nr_shards, on_each_shard_func on_each_shard);

}

-template <typename Service>
+template <typename Service, typename Traits>
template <typename... Args>
future<>
-sharded<Service>::start(Args&&... args) {
+sharded<Service, Traits>::start(Args&&... args) {
_instances.resize(smp::count);
return internal::sharded_parallel_for_each(_instances.size(),
[this, args = std::make_tuple(std::forward<Args>(args)...)] (unsigned c) mutable {
return smp::submit_to(c, [this, args] () mutable {
_instances[engine().cpu_id()].service = apply([this] (Args... args) {
@@ -546,14 +577,14 @@ sharded<Service>::start(Args&&... args) {
});
}
});
}

-template <typename Service>
+template <typename Service, typename Traits>
template <typename... Args>
future<>
-sharded<Service>::start_single(Args&&... args) {
+sharded<Service, Traits>::start_single(Args&&... args) {
assert(_instances.empty());
_instances.resize(1);
return smp::submit_to(0, [this, args = std::make_tuple(std::forward<Args>(args)...)] () mutable {
_instances[0].service = apply([this] (Args... args) {
return create_local_service(internal::unwrap_sharded_arg(std::forward<Args>(args))...);
@@ -568,109 +599,109 @@ sharded<Service>::start_single(Args&&... args) {
});
}
});
}

-template <typename Service>
+template <typename Service, typename Traits>
future<>
-sharded<Service>::stop() {
+sharded<Service, Traits>::stop() {
return internal::sharded_parallel_for_each(_instances.size(), [this] (unsigned c) mutable {
return smp::submit_to(c, [this] () mutable {
auto inst = _instances[engine().cpu_id()].service;
if (!inst) {
return make_ready_future<>();
}
- return inst->stop();
+ return Traits::stop(inst.get());
});
}).then([this] {
return internal::sharded_parallel_for_each(_instances.size(), [this] (unsigned c) {
return smp::submit_to(c, [this] {
_instances[engine().cpu_id()].service = nullptr;
return _instances[engine().cpu_id()].freed.get_future();
});
});
}).then([this] {
_instances.clear();
- _instances = std::vector<sharded<Service>::entry>();
+ _instances = std::vector<sharded::entry>();
});
}

-template <typename Service>
+template <typename Service, typename Traits>
future<>
-sharded<Service>::invoke_on_all(smp_service_group ssg, std::function<future<> (Service&)> func) {
+sharded<Service, Traits>::invoke_on_all(smp_service_group ssg, std::function<future<> (Service&)> func) {
return internal::sharded_parallel_for_each(_instances.size(), [this, ssg, func = std::move(func)] (unsigned c) {
return smp::submit_to(c, ssg, [this, func] {
return func(*get_local_service());
});
});
}

-template <typename Service>
+template <typename Service, typename Traits>
template <typename... Args>
inline
future<>
-sharded<Service>::invoke_on_all(smp_service_group ssg, future<> (Service::*func)(Args...), Args... args) {
+sharded<Service, Traits>::invoke_on_all(smp_service_group ssg, future<> (Service::*func)(Args...), Args... args) {
return invoke_on_all(ssg, invoke_on_all_func_type([func, args...] (Service& service) mutable {
return (service.*func)(args...);
}));
}

-template <typename Service>
+template <typename Service, typename Traits>
template <typename... Args>
inline
future<>
-sharded<Service>::invoke_on_all(smp_service_group ssg, void (Service::*func)(Args...), Args... args) {
+sharded<Service, Traits>::invoke_on_all(smp_service_group ssg, void (Service::*func)(Args...), Args... args) {
return invoke_on_all(ssg, invoke_on_all_func_type([func, args...] (Service& service) mutable {
(service.*func)(args...);
return make_ready_future<>();
}));
}

-template <typename Service>
+template <typename Service, typename Traits>
template <typename Func>
inline
future<>
-sharded<Service>::invoke_on_all(smp_service_group ssg, Func&& func) {
+sharded<Service, Traits>::invoke_on_all(smp_service_group ssg, Func&& func) {
static_assert(std::is_same<futurize_t<std::result_of_t<Func(Service&)>>, future<>>::value,
"invoke_on_all()'s func must return void or future<>");
return invoke_on_all(ssg, invoke_on_all_func_type([func] (Service& service) mutable {
return futurize<void>::apply(func, service);
}));
}

-template <typename Service>
+template <typename Service, typename Traits>
template <typename Func>
inline
future<>
-sharded<Service>::invoke_on_others(smp_service_group ssg, Func&& func) {
+sharded<Service, Traits>::invoke_on_others(smp_service_group ssg, Func&& func) {
static_assert(std::is_same<futurize_t<std::result_of_t<Func(Service&)>>, future<>>::value,
"invoke_on_others()'s func must return void or future<>");
return invoke_on_all(ssg, [orig = engine().cpu_id(), func = std::forward<Func>(func)] (auto& s) -> future<> {
return engine().cpu_id() == orig ? make_ready_future<>() : futurize_apply(func, s);
});
}

-template <typename Service>
-const Service& sharded<Service>::local() const {
+template <typename Service, typename Traits>
+const Service& sharded<Service, Traits>::local() const {
assert(local_is_initialized());
return *_instances[engine().cpu_id()].service;
}

-template <typename Service>
-Service& sharded<Service>::local() {
+template <typename Service, typename Traits>
+Service& sharded<Service, Traits>::local() {
assert(local_is_initialized());
return *_instances[engine().cpu_id()].service;
}

-template <typename Service>
-shared_ptr<Service> sharded<Service>::local_shared() {
+template <typename Service, typename Traits>
+shared_ptr<Service> sharded<Service, Traits>::local_shared() {
assert(local_is_initialized());
return _instances[engine().cpu_id()].service;
}

-template <typename Service>
-inline bool sharded<Service>::local_is_initialized() const {
+template <typename Service, typename Traits>
+inline bool sharded<Service, Traits>::local_is_initialized() const {
return _instances.size() > engine().cpu_id() &&
_instances[engine().cpu_id()].service;
}

/// \addtogroup smp-module
--
2.21.0

[Rafael Avila de Espindola]

Avi Kivity <a...@scylladb.com> writes:

> The sharded<Service> template was designed with cooperating service classes
> in mind. This cooperation is in the form of providing a stop() method for
> orderly shutdown.
>
> The sharded<> template is also useful for non-cooperating services: for example,
> std::unordered_map<std::string can be broadcast to all shards with
>
> sharded<std::unordered_map<sstring, sstring>> my_map;
> my_map.start(initializer).get();
> ...
> my_map.invoke_on_all([] (my_map_type& mm) { mm["x"] = "y"; }).get();
> ...
> my_map.stop().get();
>
> All is required is that the "Service" doesn't perform asynchronous operations (as
> is usual for plain data types) or that the user stops them in some other way. But
> this currently doesn't compile because the Service::stop() method is required.
>
> To allow for with, we provide a sharded_adapter<> template. If the user wraps their
> service with sharded_adapter, then stop() is no longer needed and you can distribute
> any type.
> ---
>
> Note: other ways to expose this are possible
> - detect whether Service::stop() exists and only call it if it does

This reminds me of emptyable/maybe_empty from types.hh. Isn't it simpler
overall? Or at least the complicated metaprogramming in is one place and
the sharded class template shouldn't require any changes.

Cheers,
Rafael

[Avi Kivity]

I don't like auto-detection because if someone misspells stop(), or just
forgets, then they end up with a broken thing. But maybe it's not so bad.


Both auto-detection and free-function will result in cleaner code. Let
me try free function.

[Avi Kivity]

free function is easy and nice. But now I am leaning towards
auto-detection, since it allows sharding PODs with zero effort.

[Avi Kivity]

The sharded<Service> template was designed with cooperating service classes
in mind. This cooperation is in the form of providing a stop() method for
orderly shutdown.

The sharded<> template is also useful for non-cooperating services: for example,
std::unordered_map<std::string can be broadcast to all shards with

sharded<std::unordered_map<sstring, sstring>> my_map;
my_map.start(initializer).get();
...
my_map.invoke_on_all([] (my_map_type& mm) { mm["x"] = "y"; }).get();
...
my_map.stop().get();

All is required is that the "Service" doesn't perform asynchronous operations (as
is usual for plain data types) or that the user stops them in some other way. But
this currently doesn't compile because the Service::stop() method is required.

To allow that, this patch only calls stop() if it is detected. It also adds a
customization point that allows the user to redirect stop() to a free function.
This can be useful if a class already contains a stop() that is used for something
else.
---

v2:
- replace traits class by auto-detection + explicit customization point
- unit tests

include/seastar/core/sharded.hh | 66 ++++++++++++++++++++++++++++++++-
tests/unit/sharded_test.cc | 40 ++++++++++++++++++++
2 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/include/seastar/core/sharded.hh b/include/seastar/core/sharded.hh
index 7712105d6d..d3e9f51d95 100644
--- a/include/seastar/core/sharded.hh
+++ b/include/seastar/core/sharded.hh
@@ -474,10 +474,74 @@ class sharded {
}
return inst;
}
};

+namespace internal {
+
+// Helper check if Service::stop exists
+struct sharded_has_stop {
+ // If a member names "stop" exists, try to call it, even if it doesn't
+ // have the correct signature. This is so that we don't ignore a function
+ // named stop() just because the signature is incorrect, and instead
+ // force the user to resolve the ambiguity.
+ template <typename Service>
+ constexpr static auto check(Service*) -> std::enable_if_t<(sizeof(&Service::stop) >= 0), bool> {
+ return true;
+ }
+
+ // Fallback in case Service::stop doesn't exist.
+ static constexpr auto check(...) -> bool {
+ return false;
+ }
+};
+
+template <bool stop_exists>
+struct sharded_call_stop {
+ template <typename Service>
+ static future<> call(Service& instance);
+};
+
+template <>
+template <typename Service>
+future<> sharded_call_stop<true>::call(Service& instance) {
+ return instance.stop();
+}
+
+template <>
+template <typename Service>
+future<> sharded_call_stop<false>::call(Service& instance) {
+ return make_ready_future<>();
+}
+
+}
+
+
+/// Stop a \ref sharded service instance
+///
+/// This is a customization point. The default implementation will call
+/// instance.stop() if it exists (the member function must return a future<>).
+/// If instance.stop() does not exist, the default implementation does nothing.
+///
+/// You may customize this function for your instance type by declaring an
+/// overload in the same namespace as \c Service.
+///
+/// \tparam Service a type used in a \ref sharded instance.
+/// \return a \ref future that becomes available when the instance is stopped
+/// and ready for destruction. This typically involves signalling
+/// background operations that they need to terminate, and waiting
+/// until they do.
+///
+/// \see sharded
+/// \see gate
+template <typename Service>
+future<>
+stop_sharded_instance(Service& instance) {
+ constexpr bool has_stop = internal::sharded_has_stop::check(&instance);
+ return internal::sharded_call_stop<has_stop>::call(instance);
+}
+
/// @}

template <typename Service>
sharded<Service>::~sharded() {
assert(_instances.empty());
@@ -577,11 +641,11 @@ sharded<Service>::stop() {

return smp::submit_to(c, [this] () mutable {
auto inst = _instances[engine().cpu_id()].service;
if (!inst) {
return make_ready_future<>();
}
- return inst->stop();

+ return stop_sharded_instance(*inst);

});
}).then([this] {
return internal::sharded_parallel_for_each(_instances.size(), [this] (unsigned c) {
return smp::submit_to(c, [this] {
_instances[engine().cpu_id()].service = nullptr;

diff --git a/tests/unit/sharded_test.cc b/tests/unit/sharded_test.cc
index d227840dfb..ee61555d9a 100644
--- a/tests/unit/sharded_test.cc
+++ b/tests/unit/sharded_test.cc
@@ -47,5 +47,45 @@ class invoke_on_during_stop final : public peering_sharded_service<invoke_on_dur
SEASTAR_THREAD_TEST_CASE(invoke_on_during_stop_test) {
sharded<invoke_on_during_stop> s;
s.start().get();
s.stop().get();
}
+
+namespace {
+
+struct plain_old_data {
+ int x;
+};
+
+}
+
+// Really a compile test, check that we detect that plain_old_data::stop()
+// doesn't exist and not call it.
+SEASTAR_THREAD_TEST_CASE(test_plain_old_data) {
+ sharded<plain_old_data> pod;
+ pod.start().get();
+ pod.stop().get();
+}
+
+namespace {
+
+struct custom_stop {
+ int x;
+};
+
+std::atomic<int> stop_count;
+
+future<> stop_sharded_instance(custom_stop& cs) {
+ stop_count.fetch_add(1);
+ return make_ready_future<>();
+}
+
+}
+
+// Check that we honor the stop_sharded_instance() customization point.
+SEASTAR_THREAD_TEST_CASE(test_custom_stop) {
+ sharded<custom_stop> cs;
+ cs.start().get();
+ cs.stop().get();
+ BOOST_REQUIRE_EQUAL(stop_count.load(), smp::count);
+}
+
--
2.21.0

[Avi Kivity]

Changed my mind again and went with auto detection AND free function.
Posted as v2.

[Duarte Nunes]

On 7/10/19 12:47, Avi Kivity wrote:
> The sharded<Service> template was designed with cooperating service classes
> in mind. This cooperation is in the form of providing a stop() method for
> orderly shutdown.
>
> The sharded<> template is also useful for non-cooperating services: for example,
> std::unordered_map<std::string can be broadcast to all shards with
>
> sharded<std::unordered_map<sstring, sstring>> my_map;
> my_map.start(initializer).get();
> ...
> my_map.invoke_on_all([] (my_map_type& mm) { mm["x"] = "y"; }).get();
> ...
> my_map.stop().get();
>
> All is required is that the "Service" doesn't perform asynchronous operations (as
> is usual for plain data types) or that the user stops them in some other way. But
> this currently doesn't compile because the Service::stop() method is required.
>
> To allow that, this patch only calls stop() if it is detected. It also adds a
> customization point that allows the user to redirect stop() to a free function.
> This can be useful if a class already contains a stop() that is used for something
> else.

It's a downside that it's no longer true that calling service.stop() calls the underlying type's stop() function (it's
already a bit confusing that sharded<>::start() doesn't call an underlying start() function).

Now a Service can be:

1) A class with a stop() function;
2) A class without a stop() function or anything similar;
3) A class with some other function that behaves like most stop() functions in a code base;
4) A class with a stop() function that is inconsistent with all the other stop() functions in a code base and is not
called by sharded<> (i.e., the extension point was used to call foo::close() while there exists a foo::stop());

I wonder if all this flexibility is needed? sharded<> can be opinionated about how a Service should look like, and we
can provide a wrapper type for existing, possibly library, classes:

sharded<as_service<std::unordered_map<sstring, sstring>>> my_map;

my_map.start(initializer).get();
...
my_map.invoke_on_all([] (my_map_type_to_which_as_service_converts_implicitly& mm) { mm["x"] = "y"; }).get();
...
my_map.stop().get(); // There's no underlying stop() function, but at least the type provides a clue about what's happening.

Are the two template declarations required?

[Rafael Avila de Espindola]

Avi Kivity <a...@scylladb.com> writes:


> +/// Stop a \ref sharded service instance
> +///
> +/// This is a customization point.

This feels a bit much, since it is as easy to wrap an existing class
with a new stop implementation.

I do like sharded_has_stop better than the previous patch. Would you be
OK with just

- return inst->stop();
+ constexpr bool has_stop = internal::sharded_has_stop::check(inst);

+ return internal::sharded_call_stop<has_stop>::call(instance);

LGTM with that change.

Cheers,
Rafael

[Avi Kivity]

My current use case is sharded<abort_source>. I think we also ought to
be able to support sharded<file>, which would require a future-returning
constructor. It's tempting to use a similar free function like
future<Service> start_sharded_instance(Args..) but we wouldn't want
exactly one way of starting a sharded<file> (different call sites would
want different constructors). The traits implementation gives better
options for that (see my v1).

> sharded<> can be opinionated about how a Service should look like, and
> we can provide a wrapper type for existing, possibly library, classes:
>
> sharded<as_service<std::unordered_map<sstring, sstring>>> my_map;

That's a viable alternative which I hadn't considered.


So the list of options becomes:

 0. don't do anything and force the user to provide a stop() member
(and wrap library types)

 1. traits class (my v1)

 2. auto detect stop

 3. free function customization point (inspired by
boost::intrusive_ptr_add_ref)

 4. combination of auto-detect and free-function (this patch)

 5. seastar-provided wrapper


Traits and a seastar-provided wrapper are relatively similar, traits is
easier to implement but has a minor downside of template name bloat
everywhere. I think traits is better as it also allows the user to
intercept start().


  default_sharded_traits::start(Args&&... args) { return
make_ready_future<Service>(fwd(args)...); }


  my_silly_file_sharded_traits::start(sstring fname) { return
open_file_dma(fname, some_flags); }


so between 1 and 5, I think traits are better. But then we have to
implement start() now, to avoid breaking all user-provided traits when
we do implement it, or have to perform auto-detection on traits, or
recommend that everyone inherit and override from the default traits.

>
> my_map.start(initializer).get();
> ...
> my_map.invoke_on_all([]
> (my_map_type_to_which_as_service_converts_implicitly& mm) { mm["x"] =
> "y"; }).get();
> ...
> my_map.stop().get(); // There's no underlying stop() function, but at
> least the type provides a clue about what's happening.
>

It hides much variability from the user (can be both good and bad) but
would also be moderately complex to implement, as now as_service<T>
needs to decay to T in a large number of contexts. Or alternatively we'd
need to partially specialize sharded<as<service<T>>.

Yes, it's a template member in a template struct, so both are needed.

[Avi Kivity]

I think it is too inflexible. You might want to unregister while
stopping, or something. Yes, it's easy to wrap an existing type, but
it's also awkward to use it as a sharded instance since you have to keep
unwrapping it everywhere. If you wrap with inheritance then unwrapping
is easy, but you can't always do that.

[Duarte Nunes]

No opposition to traits. (Wrapper types can also be used to intercept start calls though.)

>
>
>>
>> my_map.start(initializer).get();
>> ...
>> my_map.invoke_on_all([] (my_map_type_to_which_as_service_converts_implicitly& mm) { mm["x"] = "y"; }).get();
>> ...
>> my_map.stop().get(); // There's no underlying stop() function, but at least the type provides a clue about what's
>> happening.
>>
>
> It hides much variability from the user (can be both good and bad) but would also be moderately complex to implement,
> as now as_service<T> needs to decay to T in a large number of contexts. Or alternatively we'd need to partially
> specialize sharded<as<service<T>>.

Oh, that's true, invoke_on_all() and friends are not templated.

Ohh, had never seen that.

[Rafael Avila de Espindola]

There is always "operator inner()":

struct wrap {
inner _i;
operator const inner&() { return _i; }
...
};

Which types do you have in mind for the customization point?

Cheers,
Rafael

[Avi Kivity]

Yes, but we already have unwrap_sharded_arg(), so now it has to deal
with yet another variant.

> Which types do you have in mind for the customization point?

I don't understand the question.

[Rafael Avila de Espindola]

OK, given that there is a precedent, LGTM.

>> Which types do you have in mind for the customization point?
>
>
> I don't understand the question.

Where do you expect to use it. That is, do you have a type you want to
use the new configuration point for? Note that unwrap_sharded_arg is
defined only in sharded.hh. Are we making things more flexible than they
need to be?

Cheers,
Rafael

[Avi Kivity]

Not sure what you mean. What looks good to you?

>
>>> Which types do you have in mind for the customization point?
>>
>> I don't understand the question.
> Where do you expect to use it. That is, do you have a type you want to
> use the new configuration point for? Note that unwrap_sharded_arg is
> defined only in sharded.hh. Are we making things more flexible than they
> need to be?

We have these two data points:

 - some services can define stop()

 - some library types cannot define stop() and so must either be
wrapped by the user or by Seastar (known case exists)

 - some services could use non-atomic construction (use a function
returning a future<Service> rather than a constructor) (no known case
exists, but sounds reasonable)


Given the above, either traits or free function customization points are
needed. I'm now leaning back towards traits as less magical and less
likely to cause conflicts when two unrelated users want to shard<> a
library type (e.g. sharded<file> with different open functions). The
downside of traits is that they're cumbersome.

[Konstantin Osipov]

* Duarte Nunes <dua...@vectorized.io> [19/07/10 20:28]:

> I wonder if all this flexibility is needed? sharded<> can be opinionated
> about how a Service should look like, and we can provide a wrapper type for
> existing, possibly library, classes:
>
> sharded<as_service<std::unordered_map<sstring, sstring>>> my_map;
>
> my_map.start(initializer).get();
> ...
> my_map.invoke_on_all([] (my_map_type_to_which_as_service_converts_implicitly& mm) { mm["x"] = "y"; }).get();
> ...
> my_map.stop().get(); // There's no underlying stop() function, but at least the type provides a clue about what's happening.

An advantage of this approach is that it is more explicit
about the magic which is going on under the hood: I as a reader
can see that some adaptation is required to make an
arbitrary object sharded.

--
Konstantin Osipov, Moscow, Russia

[Rafael Avila de Espindola]

Avi Kivity <a...@scylladb.com> writes:

>>> Yes, but we already have unwrap_sharded_arg(), so now it has to deal
>>> with yet another variant.
>> OK, given that there is a precedent, LGTM.
>
>
> Not sure what you mean. What looks good to you?

Your patch. I would prefer to use wrappers, but given that there is
already unwrap_sharded_arg, there is precedent for doing what your patch
does and consistency is probably more important.

Cheers,
Rafael

[Avi Kivity]

unwrap_sharded_arg() is an internal implementation detail, I would not
use it as an argument for consistency. I brought it up because it makes
"struct wrap" more complicated, as there are several layers to unwrap.