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

refactor: connector moves and renames #110

Merged
merged 1 commit into from
Sep 29, 2022
Merged

refactor: connector moves and renames #110

merged 1 commit into from
Sep 29, 2022

Conversation

mojito317
Copy link
Contributor

@mojito317 mojito317 commented Sep 27, 2022 •
edited
Loading

Checklist

  • Tick to sign-off your agreement to the Developer Certificate of Origin (DCO) 1.1
  • Added tests for code changes or test/build only changes
  • Updated the change log file (CHANGES.md|CHANGELOG.md) or test/build only changes
  • Completed the PR template below:

Description

Related to #96

Approach

Renames under the src/main/java/com/ibm/cloud/cloudant/kafka/ path:

connect/CloudantSinkConnector.java -> SinkConnector.java
connect/CloudantSourceConnector.java -> SourceChangesConnector.java
connect/CloudantConfigValidator.java -> connect/ConfigValidator.java
connect/CloudantConnectorConfig.java -> connect/ConnectorConfig.java
connect/CloudantSinkConnectorConfig.java -> connect/SinkConnectorConfig.java
connect/CloudantSinkTask.java -> connect/SinkTask.java
connect/CloudantSinkTaskConfig.java -> connect/SinkTaskConfig.java
connect/CloudantSourceConnectorConfig.java -> connect/SourceChangesConnectorConfig.java
connect/CloudantSourceTask.java -> connect/SourceChangesTask.java
connect/CloudantSourceTaskConfig.java -> connect/SourceChangesTaskConfig.java
transforms/ArrayFlatten.java -> connect/transforms/ArrayFlatten.java
transforms/MapToStruct.java -> connect/transforms/MapToStruct.java
transforms/predicates/IsDesignDocument.java -> connect/transforms/predicates/IsDesignDocument.java

Renames under the src/test/java/com/ibm/cloud/cloudant/kafka/ path:

connect/CloudantSinkConnectorTest.java -> SinkConnectorTest.java
connect/CloudantSourceConnectorTest.java -> SourceChangesConnectorTest.java
connect/CloudantSinkErrorTest.java -> connect/SinkErrorTest.java
connect/CloudantSinkTaskTest.java -> connect/SinkTaskTest.java
connect/CloudantSourceAndSinkTest.java -> connect/SourceChangesAndCloudantSinkTest.java
connect/CloudantSourceTaskTest.java -> connect/SourceChangesTaskTest.java
transforms/ArrayFlattenTest.java -> connect/transforms/ArrayFlattenTest.java
transforms/MapToStructTest.java -> connect/transforms/MapToStructTest.java
transforms/predicates/IsDesignDocumentTest.java -> connect/transforms/predicates/IsDesignDocumentTest.java
performance/CloudantSinkPerformanceTest.java -> performance/SinkPerformanceTest.java
performance/CloudantSourceAndSinkPerformanceTest.java -> performance/SourceChangesAndSinkPerformanceTest.java
performance/CloudantSourcePerformanceTest.java -> performance/SourceChangesPerformanceTest.java

I also refactored the references in other files to reflect the new class/file/package names.

Schema & API Changes

  • "No change"

Security and Privacy

  • "No change"

Testing

  • N/A file rename/move changes

Monitoring and Logging

@mojito317 mojito317 self-assigned this Sep 27, 2022
ricellis
ricellis reviewed Sep 27, 2022
View reviewed changes
Copy link
Member

@ricellis ricellis 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 this is ok, but we will need a follow-up PR to move some of the other classes into more sensible packages too.
A couple of suggestions from me, but I think Tom has some similar thoughts and will probably give more detail.

src/main/java/com/ibm/cloud/cloudant/kafka/connect/CloudantSinkConnectorConfig.java Outdated
@@ -20,12 +20,12 @@

import java.util.Map;

public class CloudantSinkConnectorConfig extends CloudantConnectorConfig {
public class CloudantSinkConnectorConfig extends ConnectorConfig {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should probably drop the Cloudant prefix from all the Sink classes it is implicit in the package name and will give better "symmetry" with the Source connectors.

Copy link
Contributor Author

@mojito317 mojito317 Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You mean renaming CloudantSinkConnector to SinkConnector too? The problem is there's a kafka class (that this sink connector implements) named SinkConnector too: public class SinkConnector extends org.apache.kafka.connect.sink.SinkConnector should not that confuse our users?
CC @tomblench

Copy link
Contributor

Choose a reason for hiding this comment

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

Good question - personally I don't mind the name clash. Users will generally only ever use this name when they configure the connector, where they have to give the fully qualified name, eg
connector.class=com.ibm.cloud.cloudant.kafka.SinkConnector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ready in 3c79671

src/main/java/com/ibm/cloud/cloudant/kafka/connect/connectors/ChangesSourceConnector.java Outdated
@@ -11,10 +11,14 @@
* either express or implied. See the License for the specific language governing permissions
* and limitations under the License.
*/
package com.ibm.cloud.cloudant.kafka.connect;
package com.ibm.cloud.cloudant.kafka.connect.connectors;
Copy link
Member

Choose a reason for hiding this comment

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

Looking at this again it does seem a bit over-long especially as it needs entering into config.
Maybe we should drop one of hte levels here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As @tomblench suggested, I will put them on the level com.ibm.cloud.cloudant.kafka

Copy link
Contributor Author

@mojito317 mojito317 Sep 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It is fixed in 8bf0292 and tests are moved up in 59772b2

@tomblench
Copy link
Contributor

tomblench commented Sep 27, 2022 •
edited
Loading

Looks good, but I would suggest three tweaks:

  • Move the two connector classes up to com.ibm.cloud.cloudant.kafka: these are user-facing names which users will have to type, so I think they should be as short as feasibly possible
  • CloudantSinkConnector should just become SinkConnector to more closely mirror the source connector
  • The source connector should become SourceChangesConnector so that there is consistency with the sink connector, where the first word is source or sink.

In summary:
com.ibm.cloud.cloudant.kafka.SourceChangesConnector
com.ibm.cloud.cloudant.kafka.SinkConnector

@mojito317 mojito317 force-pushed the 96-connector-moves-and-renames branch from c1c10fb to 37371b0 Compare September 27, 2022 12:13
@mojito317
Copy link
Contributor Author

  • Move the two connector classes up to com.ibm.cloud.cloudant.kafka: these are user-facing names which users will have to type, so I think they should be as short as feasibly possible

This is done in 8bf0292 and tests are moved up in 59772b2

  • CloudantSinkConnector should just become SinkConnector to more closely mirror the source connector

I have not touched this yet, because of an ongoing conversation here: #110 (comment)

  • The source connector should become SourceChangesConnector so that there is consistency with the sink connector, where the first word is source or sink.

This is fixed in 59772b2

@mojito317
Copy link
Contributor Author

Thanks for the reviews @tomblench and @ricellis! I've also added a line to the CHANGES.md file, so now I'm ready to merge this PR.

@mojito317 mojito317 force-pushed the 96-connector-moves-and-renames branch from ada7563 to 88827ed Compare September 29, 2022 15:11
@mojito317 mojito317 merged commit 7a20a19 into main Sep 29, 2022
@mojito317 mojito317 deleted the 96-connector-moves-and-renames branch September 29, 2022 15:18
@ricellis ricellis added this to the 0.200.0 milestone Oct 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomblench tomblench tomblench approved these changes

@ricellis ricellis ricellis approved these changes

Assignees

@mojito317 mojito317

Labels
None yet
Projects
None yet
Milestone
0.200.0
Development

Successfully merging this pull request may close these issues.
3 participants
@mojito317 @tomblench @ricellis