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

SERVER-81719: add new kafka loader actor #1029

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

Aadeshp
Copy link
Contributor

@Aadeshp Aadeshp commented Sep 28, 2023

Thanks for submitting a PR to the Genny repo. Please include the following fields (if relevant) prior to submitting your PR.

Jira Ticket: SERVER-81719

Whats Changed:
Introducing a new actor that inserts data into a specified kafka cluster. This won't be used in DSI automatically right now since DSI doesn't support kafka at the moment, but this is moreso for local testing.

Related PRs:
vcpkg PR to install librdkafka for the genny toolchain (https://github.com/10gen/vcpkg/pull/29)

@Aadeshp Aadeshp changed the title add new kafka loader actor SERVER-81719: add new kafka loader actor Oct 3, 2023
@thessem
Copy link
Collaborator

thessem commented Oct 10, 2023

I have added myself and @PyGeek03 as reviewers on this. I have added @johndaniels and @lgfang to CC them in on this review, but they're optional (unless they are requesting changes)

@PyGeek03
Copy link
Collaborator

Hi @Aadeshp, can you update the TOOLCHAIN_BUILD_ID to be patch_c0f42baafd4845b1f77e0cc19875eba3660d64ba_651c32eca4cf47f8f46c7bd8_23_10_03_15_27_42, so that it matches the patch for your 10gen/vcpkg PR? This would enable the Genny CI tests to run.

Comment on lines +58 to +59
auto inserts = _totalInserts.start();
BOOST_LOG_TRIVIAL(debug) << " KafkaLoader Inserting " << json;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Logging might introduce unpredictable performance variance, so I think we should do any logging outside of the code path being measured as much as possible:

Suggested change
auto inserts = _totalInserts.start();
BOOST_LOG_TRIVIAL(debug) << " KafkaLoader Inserting " << json;
BOOST_LOG_TRIVIAL(debug) << " KafkaLoader Inserting " << json;
auto inserts = _totalInserts.start();

using namespace genny::testing;
namespace bson_stream = bsoncxx::builder::stream;

TEST_CASE_METHOD(KafkaTestFixture, "KafkaLoader successfully connects to a Kafka broker.", "[streams][KafkaLoader]") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

An Actor test case would only be run if one of the square-bracketed tags match one of the tags defined in a YAML file in this directory. The existing defined tags are [sharded], [single_node_replset], and [three_node_replset] ([SelfTestActor] is to test Genny code, not Actor code, so it should not be used). The square-bracketed tags are not arbitrary. So we have 2 options here:

  1. Add one of the three existing defined tags above.
  2. Add a new YAML file under the above directory to define either [streams] or [KafkaLoader].

On a related note, I found that this actor test case is also not being run because neither [streams] nor [StreamStatsReporter] are defined tags. But I think this should be fixed in another PR instead of in this PR.

@thessem
Copy link
Collaborator

thessem commented Apr 25, 2024

Hey @Aadeshp, this PR has been sitting in draft for a while, what are the plans for this one?

Copy link
Collaborator

@thessem thessem left a comment

Choose a reason for hiding this comment

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

I am significantly more onboarded with Genny than I was 7 months ago, let me know if this is something you want to push forward with by requesting a review. Otherwise I'd appreciate it if this could be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants