[PATCH 0/2] Allow reshape to be aborted

[Raphael S. Carvalho]

Now reshape can be aborted on either boot or refresh.

The workflow is:
1) reshape starts
2) user notices it's taking too long
3) nodetool stop RESHAPE

the good thing is that completed reshape work isn't lost, allowing
table to enjoy the benefits of all reshaping done up to the abortion
point.

Fixes #7738.

Raphael S. Carvalho (2):
api: make compaction manager api available earlier
compaction: Allow reshape to be aborted

api/api_init.hh | 1 +
api/api.cc | 9 ++++++++-
compaction/compaction_manager.cc | 1 +
main.cc | 3 +++
sstables/sstable_directory.cc | 8 +++++++-
5 files changed, 20 insertions(+), 2 deletions(-)

--
2.31.1

[Raphael S. Carvalho]

That will be needed for aborting reshape on boot.

Refs #7738.

Signed-off-by: Raphael S. Carvalho <raph...@scylladb.com>
---

api/api_init.hh | 1 +
api/api.cc | 9 ++++++++-

main.cc | 3 +++
3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/api/api_init.hh b/api/api_init.hh
index ecba22f17..8dce1760a 100644
--- a/api/api_init.hh
+++ b/api/api_init.hh
@@ -86,6 +86,7 @@ future<> set_server_storage_proxy(http_context& ctx);
future<> set_server_stream_manager(http_context& ctx);
future<> set_server_gossip_settle(http_context& ctx);
future<> set_server_cache(http_context& ctx);
+future<> set_server_compaction_manager(http_context& ctx);
future<> set_server_done(http_context& ctx);

}
diff --git a/api/api.cc b/api/api.cc
index bef2a736e..523ada75e 100644
--- a/api/api.cc
+++ b/api/api.cc
@@ -178,13 +178,20 @@ future<> set_server_gossip_settle(http_context& ctx) {
});
}

-future<> set_server_done(http_context& ctx) {
+future<> set_server_compaction_manager(http_context& ctx) {
auto rb = std::make_shared < api_registry_builder > (ctx.api_doc);

return ctx.http_server.set_routes([rb, &ctx](routes& r) {
rb->register_function(r, "compaction_manager",
"The Compaction manager API");
set_compaction_manager(ctx, r);
+ });
+}
+
+future<> set_server_done(http_context& ctx) {
+ auto rb = std::make_shared < api_registry_builder > (ctx.api_doc);
+
+ return ctx.http_server.set_routes([rb, &ctx](routes& r) {
rb->register_function(r, "lsa", "Log-structured allocator API");
set_lsa(ctx, r);

diff --git a/main.cc b/main.cc
index 9c102639f..364b6292c 100644
--- a/main.cc
+++ b/main.cc
@@ -1029,6 +1029,9 @@ int main(int ac, char** av) {

distributed_loader::ensure_system_table_directories(db).get();

+ // making compaction manager api available, after system keyspace has already been established.
+ api::set_server_compaction_manager(ctx).get();
+
supervisor::notify("loading non-system sstables");
distributed_loader::init_non_system_keyspaces(db, proxy, mm).get();

--
2.31.1

[Raphael S. Carvalho]

Now reshape can be aborted on either boot or refresh.

The workflow is:
1) reshape starts
2) user notices it's taking too long
3) nodetool stop RESHAPE

the good thing is that completed reshape work isn't lost, allowing
table to enjoy the benefits of all reshaping done up to the abortion
point.

Fixes #7738.

Signed-off-by: Raphael S. Carvalho <raph...@scylladb.com>
---

compaction/compaction_manager.cc | 1 +
sstables/sstable_directory.cc | 8 +++++++-
2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/compaction/compaction_manager.cc b/compaction/compaction_manager.cc
index 7f33d6e20..c3b80f2ef 100644
--- a/compaction/compaction_manager.cc
+++ b/compaction/compaction_manager.cc
@@ -317,6 +317,7 @@ future<> compaction_manager::run_custom_job(column_family* cf, sstables::compact
f.get();
} catch (sstables::compaction_stop_exception& e) {
cmlog.info("{} was abruptly stopped, reason: {}", task->type, e.what());
+ throw;
} catch (...) {
cmlog.error("{} failed: {}", task->type, std::current_exception());
throw;
diff --git a/sstables/sstable_directory.cc b/sstables/sstable_directory.cc
index 08a67e932..4d2a5b381 100644
--- a/sstables/sstable_directory.cc
+++ b/sstables/sstable_directory.cc
@@ -362,7 +362,13 @@ future<uint64_t> sstable_directory::reshape(compaction_manager& cm, table& table
return collect_output_sstables_from_reshaping(std::move(new_sstables));
});
});
- }).then([] {
+ }).then_wrapped([&table] (future<> f) {
+ try {
+ f.get();
+ } catch (sstables::compaction_stop_exception& e) {
+ dirlog.info("Table {}.{} with compaction strategy {} had reshape successfully aborted.", table.schema()->ks_name(), table.schema()->cf_name(), table.get_compaction_strategy().name());
+ return make_ready_future<stop_iteration>(stop_iteration::yes);
+ }
return make_ready_future<stop_iteration>(stop_iteration::no);
});
}).then([&reshaped_size] {
--
2.31.1

[Benny Halevy]

LGTM

[Raphael S. Carvalho]

[Avi Kivity]

On 28/07/2021 17.46, Raphael S. Carvalho wrote:
> Now reshape can be aborted on either boot or refresh.
>
> The workflow is:
> 1) reshape starts
> 2) user notices it's taking too long
> 3) nodetool stop RESHAPE
>
> the good thing is that completed reshape work isn't lost, allowing
> table to enjoy the benefits of all reshaping done up to the abortion
> point.
>
> Fixes #7738.
>
> Signed-off-by: Raphael S. Carvalho <raph...@scylladb.com>
> ---
> compaction/compaction_manager.cc | 1 +
> sstables/sstable_directory.cc | 8 +++++++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/compaction/compaction_manager.cc b/compaction/compaction_manager.cc
> index 7f33d6e20..c3b80f2ef 100644
> --- a/compaction/compaction_manager.cc
> +++ b/compaction/compaction_manager.cc
> @@ -317,6 +317,7 @@ future<> compaction_manager::run_custom_job(column_family* cf, sstables::compact
> f.get();
> } catch (sstables::compaction_stop_exception& e) {
> cmlog.info("{} was abruptly stopped, reason: {}", task->type, e.what());
> + throw;

How does this throw affect non-reshape compaction?

[Raphael S. Carvalho]

The other user is resharding. Resharding can be stopped[1], only on
refresh (that's because compaction api isn't available in early boot
phase until this series), after 6ff8bb4eac662d, and when that happens,
manager fails to propagate exception upwards, and table won't allow a
shared sstable to even reach the sstable set, as expected
(https://github.com/scylladb/scylla/blob/next/table.cc#L341). From now
on, manager will propagate the exception and the difference is that
refresh will fail with the stop exception, so the user will have to
rerun the command.

[1]: nodetool stop cannot be used for that though as RESHARD isn't a
recognized type, so one would have to explicitly use HTTP api to
achieve that. Along with this series, nodetool was patched to
recognize the RESHAPE type.

[Avi Kivity]

Can't it happen to any compaction?

> Resharding can be stopped[1], only on
> refresh (that's because compaction api isn't available in early boot
> phase until this series), after 6ff8bb4eac662d, and when that happens,
> manager fails to propagate exception upwards, and table won't allow a
> shared sstable to even reach the sstable set, as expected
> (https://github.com/scylladb/scylla/blob/next/table.cc#L341). From now
> on, manager will propagate the exception and the difference is that
> refresh will fail with the stop exception, so the user will have to
> rerun the command.

What happens if an ordinary compaction is stopped like that?


Propagating the exception seems more correct than not, but I'd like to
understand if there was a reason to swallow the exception originally.

[Raphael S. Carvalho]

no, regular, cleanup, go through their own specialized functions and
they share this common handler
https://github.com/scylladb/scylla/blob/next/compaction/compaction_manager.cc#L525.
run_custom_job() is limited to external users like resharding and
reshape, which can specify a custom job to run through the manager. In
my opinion, this is all a bit messy and I want to refactor exception
handling there soon.

>
>
> > Resharding can be stopped[1], only on
> > refresh (that's because compaction api isn't available in early boot
> > phase until this series), after 6ff8bb4eac662d, and when that happens,
> > manager fails to propagate exception upwards, and table won't allow a
> > shared sstable to even reach the sstable set, as expected
> > (https://github.com/scylladb/scylla/blob/next/table.cc#L341). From now
> > on, manager will propagate the exception and the difference is that
> > refresh will fail with the stop exception, so the user will have to
> > rerun the command.
>
>
> What happens if an ordinary compaction is stopped like that?

compaction is retried if stopped using exponential backoff retry
logic, not necessarily with the same set of candidates though. a new
input set is picked from strategy when the time for retry comes.

>
>
> Propagating the exception seems more correct than not, but I'd like to
> understand if there was a reason to swallow the exception originally.

Probably a bug. Initially only asynchronous resharding was using it,
so it wasn't an issue. System could also work with shared sstables.
Now with synchronous resharding and reshape, propagating the stop
exception is the right thing to do.

[Avi Kivity]

Please post a git url.

On 28/07/2021 17.46, Raphael S. Carvalho wrote:

[Raphael S. Carvalho]

On Mon, Aug 2, 2021 at 11:17 AM Avi Kivity <a...@scylladb.com> wrote:
>
> Please post a git url.

https://github.com/raphaelsc/scylla.git abort_reshape_v1

[Commit Bot]

From: Raphael S. Carvalho <raph...@scylladb.com>
Committer: Raphael S. Carvalho <raph...@scylladb.com>
Branch: next

api: make compaction manager api available earlier

That will be needed for aborting reshape on boot.

Refs #7738.

Signed-off-by: Raphael S. Carvalho <raph...@scylladb.com>

---

diff --git a/api/api.cc b/api/api.cc
--- a/api/api.cc
+++ b/api/api.cc
@@ -182,13 +182,20 @@ future<> set_server_gossip_settle(http_context& ctx) {

});
}

-future<> set_server_done(http_context& ctx) {
+future<> set_server_compaction_manager(http_context& ctx) {
auto rb = std::make_shared < api_registry_builder > (ctx.api_doc);

return ctx.http_server.set_routes([rb, &ctx](routes& r) {
rb->register_function(r, "compaction_manager",
"The Compaction manager API");
set_compaction_manager(ctx, r);
+ });
+}
+
+future<> set_server_done(http_context& ctx) {
+ auto rb = std::make_shared < api_registry_builder > (ctx.api_doc);
+
+ return ctx.http_server.set_routes([rb, &ctx](routes& r) {
rb->register_function(r, "lsa", "Log-structured allocator API");
set_lsa(ctx, r);

diff --git a/api/api_init.hh b/api/api_init.hh
--- a/api/api_init.hh
+++ b/api/api_init.hh
@@ -87,6 +87,7 @@ future<> set_server_storage_proxy(http_context& ctx, sharded<service::storage_se

future<> set_server_stream_manager(http_context& ctx);
future<> set_server_gossip_settle(http_context& ctx);
future<> set_server_cache(http_context& ctx);
+future<> set_server_compaction_manager(http_context& ctx);
future<> set_server_done(http_context& ctx);

}

diff --git a/main.cc b/main.cc
--- a/main.cc
+++ b/main.cc
@@ -1031,6 +1031,9 @@ int main(int ac, char** av) {

[Commit Bot]

From: Raphael S. Carvalho <raph...@scylladb.com>
Committer: Raphael S. Carvalho <raph...@scylladb.com>
Branch: next

compaction: Allow reshape to be aborted

Now reshape can be aborted on either boot or refresh.

The workflow is:
1) reshape starts
2) user notices it's taking too long
3) nodetool stop RESHAPE

the good thing is that completed reshape work isn't lost, allowing
table to enjoy the benefits of all reshaping done up to the abortion
point.

Fixes #7738.

Signed-off-by: Raphael S. Carvalho <raph...@scylladb.com>

---

diff --git a/compaction/compaction_manager.cc b/compaction/compaction_manager.cc

--- a/compaction/compaction_manager.cc
+++ b/compaction/compaction_manager.cc
@@ -317,6 +317,7 @@ future<> compaction_manager::run_custom_job(column_family* cf, sstables::compact
f.get();
} catch (sstables::compaction_stop_exception& e) {
cmlog.info("{} was abruptly stopped, reason: {}", task->type, e.what());
+ throw;

} catch (...) {
cmlog.error("{} failed: {}", task->type, std::current_exception());
throw;
diff --git a/sstables/sstable_directory.cc b/sstables/sstable_directory.cc

[Commit Bot]

From: Raphael S. Carvalho <raph...@scylladb.com>
Committer: Raphael S. Carvalho <raph...@scylladb.com>

Branch: master

[Commit Bot]

From: Raphael S. Carvalho <raph...@scylladb.com>
Committer: Raphael S. Carvalho <raph...@scylladb.com>
Branch: master

api: make compaction manager api available earlier

That will be needed for aborting reshape on boot.

Refs #7738.

Signed-off-by: Raphael S. Carvalho <raph...@scylladb.com>

---

[Benny Halevy]

Propagating the exception caused a regression
when scylla shuts down distributed_loader::populate_column_family
(https://github.com/scylladb/scylla/issues/9158)

We need to handle the propagated exception
for clean shutdown.