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

KafkaConnect code base refactor #709

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Conversation

worryg0d
Copy link
Collaborator

No description provided.

@worryg0d worryg0d added the refactor Code improvements or refactorings label Feb 14, 2024
@worryg0d worryg0d self-assigned this Feb 14, 2024
@worryg0d worryg0d force-pushed the kafka-connect-code-base-refactor branch from db62598 to d28f2e0 Compare February 14, 2024 12:39
type KafkaConnectDataCentreStatus struct {
GenericDataCentreStatus `json:",inline"`

NumberOfNodes int `json:"numberOfNodes,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

please try to make it generic

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A bit hard because some resources don't have any numberOfNodes inside their dataCentres so if we add the field to the generic status it will exist inside the resource status but never filled. The only way to implement it for such resources is to count it on my own. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should count it on our end. Also, I'm considering moving this field into the 'Status,' which allows us to use the print column annotations (for convenient displaying of the total nodes number) and simplifies the calculation when dealing with multiple data centers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines 226 to 236
return k.GenericDataCentreSpec.Equals(&o.GenericDataCentreSpec) &&
k.NumberOfNodes == o.NumberOfNodes &&
k.ReplicationFactor == o.ReplicationFactor
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nodeSize is missing

settings := cc.GCPConnectorSettings[i]
if *gSetting != *settings {
for i := range ks.CustomConnectors {
if !slices.Equals(ks.CustomConnectors[i].AWSConnectorSettings, o[i].AWSConnectorSettings) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

EqualsPtr

Comment on lines 196 to 206
func (ks *KafkaConnectSpec) FromInstAPI(instaModel *models.KafkaConnectCluster) {
ks.GenericClusterSpec.FromInstAPI(&instaModel.GenericClusterFields)
ks.DCsFromInstAPI(instaModel.DataCentres)
ks.TargetClustersFromInstAPI(instaModel.TargetCluster)
ks.CustomConnectorsFromInstAPI(instaModel.CustomConnectors)
Copy link
Contributor

Choose a reason for hiding this comment

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

version is missing


func (s *KafkaConnectDataCentreStatus) FromInstAPI(instaModel *models.KafkaConnectDataCentre) {
s.GenericDataCentreStatus.FromInstAPI(&instaModel.GenericDataCentreFields)
s.nodesFromInstAPI(instaModel.Nodes)
Copy link
Contributor

Choose a reason for hiding this comment

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

try to make nodesFromInstAPI generic

Comment on lines 143 to 149
managedCluster := kc.Spec.GetManagedCluster()

kc.Spec.FromInstAPI(&instaModel)

if managedCluster != nil && managedCluster.ClusterRef != nil {
kc.Spec.GetManagedCluster().ClusterRef = managedCluster.ClusterRef
}
Copy link
Contributor

Choose a reason for hiding this comment

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

move managedCluster into FromInstAPI

@worryg0d worryg0d force-pushed the kafka-connect-code-base-refactor branch 2 times, most recently from e0df0d7 to e76f27a Compare February 19, 2024 09:07
@worryg0d worryg0d force-pushed the kafka-connect-code-base-refactor branch from e76f27a to 49d0b34 Compare February 19, 2024 13:44
cs.DataCentres = dcs
}

func (c *CassandraSpec) getDC(name string) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about naming the function such as GetDCIndexByName?

@@ -179,6 +175,10 @@ func (kcv *kafkaConnectValidator) ValidateUpdate(ctx context.Context, old runtim

kafkaconnectlog.Info("validate update", "name", kc.Name)

if kc.Annotations[models.ResourceStateAnnotation] == models.CreatingEvent {
Copy link
Contributor

Choose a reason for hiding this comment

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

SyncEvent

spec:
name: "example-cassandra" #(immutable)
name: "bohdan-cassandra" #(immutable)
Copy link
Contributor

Choose a reason for hiding this comment

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

revert

Comment on lines 38 to 41
"sigs.k8s.io/controller-runtime/pkg/ratelimiter"

rlimiter "github.com/instaclustr/operator/pkg/ratelimiter"

"github.com/instaclustr/operator/apis/clusters/v1beta1"
"github.com/instaclustr/operator/pkg/exposeservice"
Copy link
Contributor

Choose a reason for hiding this comment

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

no need for a gap

Comment on lines 114 to 116
return fmt.Errorf("failed to get managed cluster id by ref %s/%s for kind %s, err: %w",
managedCluster.ClusterRef.Namespace,
managedCluster.ClusterRef.Name,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use here AsNamespaced().String

@@ -655,6 +620,10 @@ func (r *KafkaConnectReconciler) SetupWithManager(mgr ctrl.Manager) error {
return false
}

if newObj.Status.ID == "" && newObj.Annotations[models.ResourceStateAnnotation] == models.CreatingEvent {
Copy link
Contributor

Choose a reason for hiding this comment

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

SyncEvent

@worryg0d worryg0d force-pushed the kafka-connect-code-base-refactor branch from 49d0b34 to 1efbe0c Compare February 20, 2024 12:40
@worryg0d worryg0d requested a review from ribaraka February 20, 2024 12:42
@ribaraka ribaraka merged commit 0869e02 into main Feb 21, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code improvements or refactorings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants