schema: column_mapping::{static,regular}_column_at(): use on_internal_error() #17080
Conversation
To faciliate further patching.
…_error() Instead of std::out_of_range(). Accessing a non-existing column is a serious bug and the backtrace coming with on_internal_error() can be very valuable when debugging it. As can be the coredump that is possible to trigger with --abort-on-internal-error. This change follows another similar change to schema::column_at().
denesb
force-pushed
the
column-mapping-at-internal-error
branch
from
5aa7f41
to
ecf654e
Compare
| @@ -94,15 +94,15 @@ const column_mapping_entry& column_mapping::column_at(column_kind kind, column_i | |||
|
|
|||
| const column_mapping_entry& column_mapping::static_column_at(column_id id) const { | |||
| if (id >= _n_static) { | |||
| throw std::out_of_range(format("static column id {:d} >= {:d}", id, _n_static)); | |||
| on_internal_error(dblog, format("static column id {:d} >= {:d}", id, _n_static)); | |||
There was a problem hiding this comment.
Just yesterday I had a similar problem: I wanted to add a call to on_internal_error() inside a header file, and realized that I can't, because it requires a logger and you don't have one in a header file.
Instead of moving code out of the header file (which can have performance implications, which you probably didn't measure) just to please on_internal_error, I propose that we had a new utils::on_internal_error() function in Scylla (not in Seastar) which uses one specific logger (e.g., internal_error_logger) for all of internal errors. Then we can use this new on_internal_error in an existing header file.
(however, maybe there's reason to move this stuff out of the header file in any case - e.g., issue #1 - but if there is, please mention it. If you only moved it because of on_internal_error, I'm suggesting here an alternative that can be useful in the future in many places).
There was a problem hiding this comment.
The similar schema::column_at() method is also in schema.cc, so I followed suit. I think there is no reason for this to be in a header file, it was probably just convenient to leave it there.
There was a problem hiding this comment.
Ok, sounds good, but just tell me if I guessed correctly, and the reason why you had to move it in this patch was your sudden desire for symmetry with schema, but the on_internal_error() logger? :-)
Hopefully the new utils::on_internal_error() that I'm proposing will make people less reluctant to use on_internal_error() everywhere.
🟢 CI State: SUCCESS✅ - Build Build Details:
|
Instead of std::out_of_range(). Accessing a non-existing column is a serious bug and the backtrace coming with
on_internal_error()can be very valuable when debugging it. As can be the coredump that is possibleto trigger with
--abort-on-internal-error.This change follows another similar change to
schema::column_at().This should help us get to the bottom of the mysterious repair failures caused by invalid column access, seen in #16821.
Refs: #16821