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

make si mode two separate flags #4812

Closed
wants to merge 10 commits into from
Closed

Conversation

LenaAn
Copy link
Contributor

@LenaAn LenaAn commented May 19, 2022

Will split into commits that build later.
Also need to think about compatibility

Fixes: #4499

@LenaAn
Copy link
Contributor Author

LenaAn commented May 20, 2022

This change breaks forward compatibility.

Also I think it's better to base it on top of @mmaslankaprv 's change on Serde

@LenaAn
Copy link
Contributor Author

LenaAn commented May 20, 2022

Currently rpk topic create doesn't work since it passes shadow_indexing: {nullopt}

@@ -397,11 +399,12 @@ struct property_update<tristate<T>> {
struct incremental_topic_updates {
static constexpr int8_t version_with_data_policy = -1;
static constexpr int8_t version_with_shadow_indexing = -3;
static constexpr int8_t version_with_separate_si_flags = -4;
// negative version indicating different format:
// -1 - topic_updates with data_policy
// -2 - topic_updates without data_policy
// -3 - topic_updates with shadow_indexing
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: comment about version -4?

Copy link
Contributor

Choose a reason for hiding this comment

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

on the other hand, the names are self descriptive

return;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

that was genuinely terrible, good riddance

Lazin
Lazin previously approved these changes May 20, 2022
Copy link
Contributor

@Lazin Lazin left a comment

Choose a reason for hiding this comment

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

LGTM, the only reservation that I have is that we might soon transition to serde so the serialisation of this commands might be done much simpler. In this form it breaks forward compatibility. We won't be able to roll back redpanda to previous version if at least one topic is created.

@jcsp
Copy link
Contributor

jcsp commented Jun 6, 2022

It's not obvious to me why the on-disk encoding needs to change: it was already storing two bits of information (packed into shadow_indexing_mode), so surely it can store the same thing as two boolean attributes stores? Couldn't we just do some logic in the encode/decode path that changes converts from "two bools" into "value of shadow_indexing_mode"?

@LenaAn
Copy link
Contributor Author

LenaAn commented Jun 6, 2022

It's not obvious to me why the on-disk encoding needs to change: it was already storing two bits of information (packed into shadow_indexing_mode), so surely it can store the same thing as two boolean attributes stores? Couldn't we just do some logic in the encode/decode path that changes converts from "two bools" into "value of shadow_indexing_mode"?

@jcsp you mean not to change topic_properties, but to change adl<cluster::topic_configuration>::to( iobuf& out, cluster::topic_configuration&& t) and cluster::topic_configuration adl<cluster::topic_configuration>::from(iobuf_parser& in)?

What about incremental_topic_update? Currently the cause of the bug is this function assumes that only either read or write can be set/removed. https://github.com/redpanda-data/redpanda/blob/dev/src/v/cluster/topic_table.cc#L295

We actually have {read_enabled, read_disabled} * {write_enabled, write_disabled} = 4 states and {set_read, drop_read} * {set_write, drop_write} = 4 operation and try to encode that in 7 values of shadow_indexing_mode enum.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to set redpanda.remote.write and redpanda.remote.read together in a single rpk command
3 participants