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.

Sign up for GitHub

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

Jump to bottom

Using connector name as consumer group #1109

Merged
merged 3 commits into from
Apr 10, 2024
Merged

Using connector name as consumer group #1109

merged 3 commits into from
Apr 10, 2024

Conversation

GoMati-MU
Copy link
Contributor

Description

This short PR is to use Connector's Name (picked from config name) as consumer group for Kafka Consumer instead of relying on user to provide it.

stheppi
stheppi requested changes Apr 8, 2024
View reviewed changes
Copy link
Contributor

@stheppi stheppi left a comment

Choose a reason for hiding this comment

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

I think there is more to this PR to be done.

The connector code appends the Kafka consumer settings with a prefix. That code needs to be updated to drop the group.id setting such that it's not even a setting that the user can set.

@GoMati-MU
Copy link
Contributor Author

I think there is more to this PR to be done.

The connector code appends the Kafka consumer settings with a prefix. That code needs to be updated to drop the group.id setting such that it's not even a setting that the user can set.

Unfortunately I cannot reply directly under comment so I reply here.

group.id was not added to Connector's ConfigDef as it was one of the passthrough configs (we were basically stripping prefix from it) so we don't have to delete it from there.
Basically what needs to be done is to add another parameter (group.id) to documentation to indicate to user that it's being specified by the connector and that it's being ignored and not being passed through.

Happy to discuss if needed

GoMati-MU added 2 commits April 9, 2024 09:38
@GoMati-MU
not exposing ignored ConsumerProperties in ConfigDef
f7974a9 
not exposing ignored ConsumerProperties in ConfigDef (as connector sets
it by itself)
@GoMati-MU
file movements
23c8f74
@GoMati-MU
Copy link
Contributor Author

I think there is more to this PR to be done.
The connector code appends the Kafka consumer settings with a prefix. That code needs to be updated to drop the group.id setting such that it's not even a setting that the user can set.

Unfortunately I cannot reply directly under comment so I reply here.

group.id was not added to Connector's ConfigDef as it was one of the passthrough configs (we were basically stripping prefix from it) so we don't have to delete it from there. Basically what needs to be done is to add another parameter (group.id) to documentation to indicate to user that it's being specified by the connector and that it's being ignored and not being passed through.

Happy to discuss if needed

After discussion we've decided to remove them also from ConfigDef.

@GoMati-MU GoMati-MU merged commit 0e742b2 into feat/java-azure-connector Apr 10, 2024
127 of 130 checks passed
@GoMati-MU GoMati-MU deleted the feat/LC-173-connector-name-in-groupid branch April 10, 2024 10:30
GoMati-MU added a commit that referenced this pull request Apr 23, 2024
@GoMati-MU @murban-ziflow @davidsloan
Add Java Connectors (with EventHubs) to master (#1154)
5e77256 
* adding Azure EventHubs connector skeleton with config

* Azure EventHubs Connector MVP (#1033)

Adds Azure EventHubs Connector MVP over Kafka API.

Commits:

* Azure Connector MVP

* added prefix to Config

* adressing PR comments pt. 1

* adressing PR comments pt. 2

* addressing PR comments pt. 3

* removed unused files

* addressing PR comments pt. 4

* small log issue, getting rid of unused singleton classes

* Build KCQL from gradle (#1042)

* adding Gradle buildscripts for KCQL

* LC-128 adding KCQL to Azure EventHubs and additional properties to Azure

* removing mongodb module (it's scrapped)

* added org.antlr.* package shadowing to avoid conflicts in kafka connect

* adressing PR comments, fixing unit tests

* Azure Eventhubs bytes passthrough (#1047)

* Abstracting data that's being passed through connector to Bytes

* cleanup, additional tests

* picking the first KCQL (in case there are more)

* delegated KCQL properties handling to ProducerProvider

* PR comments

* PR comments pt.2

* added java-specific .gitignore

* missing import

* renaming producer to better indicate its purpose

* Lc 136 eventhubs get output input topics from kcql (#1058)

* getting input/output from KCQL initial impl

* removing small issues (we don't need deserializers configs)

* codacy check

* addressing PR comments pt.1

* cleanup of multiple output topics

* remove 'java' part from package name (#1103)

* remove 'java' part from package name

* comments and remarks pt. 1

* eventub connector to support multiple kcqls (#1108)

* support multiple KCQLs

* remove 'java' part from package name

* merged latest changes, added multi-KCQL support

* fixing bug aroud regex check for topic name, added unit tests

* build scripts (in order to merge PRs)

* additional tests

* Added KCQL parsing and validation to Connector class

* addressing PR comments pt. 1

* Using connector name as consumer group (#1109)

* Using connector name as consumer group

* not exposing ignored ConsumerProperties in ConfigDef

not exposing ignored ConsumerProperties in ConfigDef (as connector sets
it by itself)

* file movements

* GitHub actions for java connectors (#1147)

* preparing modules list in Gradle

* addung workflow for build scripts

* adding Java release pipeline, small regex changes

* JDK to 17 for full-support

* pushing version to 6.4.0-SNAPSHOT to keep them in sync with sbt

* add dependency check to java build workflow

* removed KCQL from java release (as it's built by sbt)

* removed unnecessary parameter

* fix dependency check

* push java test results to subdirectory

* run build on push branch

* run actions on push

* remove build workflow run on every push

* fix github warnings for deprecated functions

* addressing java-matrix output being reset with scala artifacts

* creating separate output from build script 'java_modules'

* Feat/java license headers (#1160)

* add java headers config

* add license headers to source files

* add missing HEADER file

* this commit should fail (no header in one of the tests)

* fixing commit plus including info about check in github actions

* addressing PR comments pt. 1

* PR comments pt.2

* added assertj dependancy for unit tests

* fix build script

* fixed failing test

* addressing PR comments pt.3

* adressed PR comments pt.4

* adressed PR comments pt.5

* Refactoring for simplicity (#1170)

* Refactoring for simplicity
Method Extraction: Extracted repetitive setup code into helper methods like mockKeyValueTypes, mockByteBlockingProducer, mockConsumerRecord, mockEmptyHeaders, and mockRecordsQueue.
Parameterized Methods: Made the mockRecordsQueue method varargs to accept variable numbers of input topics for better reusability.
Local Variable Removal: Removed unnecessary local variables like emptyHeaderList and directly invoked Collections.emptyIterator() where needed.
Inline Initialization: Initialized recordsQueue directly in the test methods instead of setting it up in setUp method, as it's only used within the scope of those methods.
Stream Processing: Utilized Java Streams to simplify the creation of consumerRecordList in the mockRecordsQueue method.
Simplified Assertions: Removed unnecessary assertions and verifications to reduce clutter and improve readability.

* small optimizations (imports, constants, formatting)

---------

Co-authored-by: Mati Urban <[email protected]>

---------

Co-authored-by: Mati Urban <[email protected]>
Co-authored-by: David Sloan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stheppi stheppi stheppi requested changes

@cmcmteixeira cmcmteixeira cmcmteixeira approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GoMati-MU @stheppi @cmcmteixeira