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

treewide: Reduce header dependencies #17002

Merged
merged 15 commits into from
Mar 16, 2024

Conversation

BenPope
Copy link
Member

@BenPope BenPope commented Mar 11, 2024

This focuses primarily on header files. The goal was to reduce the header dependencies between raft & cluster and other libraries such as kafka.

Another effort in another PR can focus on removing unused includes from .cc files.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@BenPope BenPope changed the title cluster:raft: Reduce header dependencies cluster|raft: Reduce header dependencies Mar 11, 2024
@rockwotj
Copy link
Contributor

Wow thanks for slogging through this 😍

@github-actions github-actions bot added the area/wasm WASM Data Transforms label Mar 11, 2024
@BenPope BenPope changed the title cluster|raft: Reduce header dependencies treewide: Reduce header dependencies Mar 11, 2024
@redpanda-data redpanda-data deleted a comment from vbotbuildovich Mar 12, 2024
@redpanda-data redpanda-data deleted a comment from vbotbuildovich Mar 12, 2024
@mmaslankaprv
Copy link
Member

thank you for that Ben 🙇🏼‍♂️

Copy link
Contributor

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

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

Could you explain 58505b3 a little more? Is this used anywhere?

It just feels a little out of place compared to other changes.

@BenPope
Copy link
Member Author

BenPope commented Mar 12, 2024

Could you explain 58505b3 a little more? Is this used anywhere?

It just feels a little out of place compared to other changes.

Yes of course, this is the best commit!

So cluster::partition includes a fair bit of code. By allowing the type to be incomplete, it allows us to not have to include the file until the lw_shared_ptr is destructed. So I use a little trick:

  1. Declare the destructor of holder of the object out of line, e.g.:
    a. kafka::group
    b. kafka::group_manager::attached_partition
  2. Move #include "cluster/partition.h" from the header to the implementation file

Now, users of kafka::replicated_partition are completely unaware that it is backed by cluster, since that is an implementation detail.

@BenPope BenPope requested a review from rockwotj March 12, 2024 12:26
rockwotj
rockwotj previously approved these changes Mar 12, 2024
Copy link
Contributor

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

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

This LGTM, except that kafka_server_rpfixture is failing for some reason.

@emaxerrno
Copy link
Contributor

@BenPope this is super cool. prolly worth a training chat on tuesday w/ the team

dotnwat
dotnwat previously approved these changes Mar 16, 2024
Copy link
Member

@dotnwat dotnwat left a comment

Choose a reason for hiding this comment

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

the lords work

Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
Signed-off-by: Ben Pope <ben@redpanda.com>
@BenPope BenPope dismissed stale reviews from dotnwat and rockwotj via cf3f1d8 March 16, 2024 17:01
@BenPope
Copy link
Member Author

BenPope commented Mar 16, 2024

Changes in force push

@BenPope BenPope merged commit 0ecbec8 into redpanda-data:dev Mar 16, 2024
17 checks passed
Comment on lines +23 to +46
enum class consistency_level { quorum_ack, leader_ack, no_ack };

struct replicate_options {
explicit replicate_options(consistency_level l)
: consistency(l)
, timeout(std::nullopt) {}

replicate_options(consistency_level l, std::chrono::milliseconds timeout)
: consistency(l)
, timeout(timeout) {}

consistency_level consistency;
std::optional<std::chrono::milliseconds> timeout;
};

struct replicate_result {
/// used by the kafka API to produce a kafka reply to produce request.
/// see produce_request.cc
model::offset last_offset;
};

struct replicate_stages {
replicate_stages(ss::future<>, ss::future<result<replicate_result>>);
explicit replicate_stages(raft::errc);
Copy link
Member

Choose a reason for hiding this comment

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

yeh this is great

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/redpanda area/wasm WASM Data Transforms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants