Cassandra codebase refactor #697

worryg0d · 2024-02-02T14:33:17Z

This PR provides refactor of Cassandra CR code base.

DoodgeMatvey · 2024-02-02T14:54:20Z

apis/clusters/v1beta1/cassandra_types.go

+func (s *CassandraDataCentreStatus) FromInstAPI(instModel *models.CassandraDataCentre) {
+	s.GenericDataCentreStatus.FromInstAPI(&instModel.GenericDataCentreFields)
+
+	s.Nodes = make([]*Node, 0, len(instModel.Nodes))
+	for _, instNode := range instModel.Nodes {
+		node := &Node{}
+		node.FromInstAPI(instNode)
+		s.Nodes = append(s.Nodes, node)
+	}
+}


I think you can remove 0, from make.
Also, if you want to preallocate the slice based on the number of nodes it is also good to use direct assignment (s.Nodes[i] = node) instead of appending, you leverage the fact that the slice is preallocated. This is a bit more efficient and arguably simpler, because it avoids the overhead of slice append operations

Actually it makes sense. Changed it with index operator [] semathic.

DoodgeMatvey · 2024-02-02T14:58:59Z

apis/clusters/v1beta1/cassandra_types.go

-	RestoreFrom         *CassandraRestoreFrom `json:"restoreFrom,omitempty"`
-	OnPremisesSpec      *OnPremisesSpec       `json:"onPremisesSpec,omitempty"`
-	Cluster             `json:",inline"`
+	GenericClusterSpec `json:",inline"`


Typically, ClusterSpec format is used, it is clear and avoid the redundancy of "generic".
The same for ClusterStatus etc.

It is not available to do this thing with ClusterStatus because it still exists in the code. So for now I want to keep it as it is. When we refactor all the cluster resources to this pattern it will be changed too

DoodgeMatvey · 2024-02-02T15:18:55Z

apis/clusters/v1beta1/validation.go

+	if s.CloudProvider != models.ONPREMISES {
+		return fmt.Errorf("cloud provider %s is unavailable for data centre: %s, available value: %s",
+			s.CloudProvider, s.Name, models.ONPREMISES)
+	}
+
+	if s.Region != models.CLIENTDC {
+		return fmt.Errorf("region %s is unavailable for data centre: %s, available value: %s",
+			s.Region, s.Name, models.CLIENTDC)
+	}


Maybe it is better to make it as 1 check and then write available values in return?

The returning errors are a bit difference so I think the following impl is better.

DoodgeMatvey · 2024-02-02T15:25:43Z

controllers/clusters/cassandra_controller.go

+	)
+
+	ctx := context.Background()
+
+	err = handleCreateOnPremisesClusterResources(ctx, bootstrap)


You can just put ctx as an func argument and use it

DoodgeMatvey · 2024-02-02T15:28:36Z

controllers/clusters/cassandra_controller.go

+	if c.Status.State != models.DeletedStatus {
+		err := r.startClusterJobs(c, l)
+		if err != nil {
+			return reconcile.Result{}, fmt.Errorf("failed to start cluster jobs, err: %w", err)
+		}
+	}
+
+	if c.Spec.OnPremisesSpec != nil && c.Spec.OnPremisesSpec.EnableAutomation {
+		return r.handleOnPremises(c, l)
+	}
+
+	return models.ExitReconcile, nil
+}


So if cluster status is deleted. With your logic it will just continue executing, right?

When I started refactoring I decided to try to keep flow as it has been before. Anyway I agree it looks a bit strange and there is cassandra details api call inside handleOnPremises method.
I decided to move it inside the if statement above.

DoodgeMatvey · 2024-02-05T10:47:53Z

apis/clusters/v1beta1/structs.go

+	*g = make(GenericResizeSettings, 0, len(instModels))
+	for _, instModel := range instModels {
+		*g = append(*g, &ResizeSettings{
+			NotifySupportContacts: instModel.NotifySupportContacts,
+			Concurrency:           instModel.Concurrency,
+		})
+	}
+}
+
+func (g *GenericResizeSettings) ToInstAPI() []*models.ResizeSettings {
+	instaModels := make([]*models.ResizeSettings, 0, len(*g))
+	for _, setting := range *g {
+		instaModels = append(instaModels, &models.ResizeSettings{
+			NotifySupportContacts: setting.NotifySupportContacts,
+			Concurrency:           setting.Concurrency,
+		})
+	}
+


Please remove 0 from make here as you did in other cases

ribaraka · 2024-02-05T11:11:55Z

apis/clusters/v1beta1/cassandra_types.go

+}
+
+func (cs *CassandraDataCentre) debeziumFromInstAPI(instModels []*models.Debezium) {
+	cs.Debezium = make([]DebeziumCassandraSpec, 0, len(instModels))


improve the slice preallocation

ribaraka · 2024-02-05T11:12:01Z

apis/clusters/v1beta1/cassandra_types.go

-			Debezium:                       cs.DebeziumFromInstAPI(iDC.Debezium),
-			ShotoverProxy:                  cs.ShotoverProxyFromInstAPI(iDC.ShotoverProxy),
+func (cs *CassandraDataCentre) shotoverProxyFromInstAPI(instModels []*models.ShotoverProxy) {
+	cs.ShotoverProxy = make([]ShotoverProxySpec, 0, len(instModels))


ribaraka · 2024-02-05T11:26:00Z

apis/clusters/v1beta1/validation.go

+	return nil
+}
+
+func (s *GenericDataCentreSpec) validateImmutableCloudProviderSettingsUpdate(oldSettings []*CloudProviderSettings) error {


this fnc is similar to equals funcs

ribaraka · 2024-02-05T11:29:26Z

controllers/clusters/cassandra_controller.go

+	for i, dc := range c.Spec.DataCentres {
+		for j, debezium := range dc.Debezium {
+			if debezium.ClusterRef != nil {
+				cdcID, err := clusterresources.GetDataCentreID(r.Client, ctx, debezium.ClusterRef)
+				if err != nil {
+					l.Error(err, "Cannot get cluster ID",
+						"Cluster reference", debezium.ClusterRef,
+					)
+					return nil, err
+				}
+				iCassandraSpec.DataCentres[i].Debezium[j].KafkaDataCentreID = cdcID
+			}
 		}
+	}


please make a func of it

ribaraka · 2024-02-05T11:34:16Z

controllers/clusters/cassandra_controller.go

+func (r *CassandraReconciler) startClusterJobs(c *v1beta1.Cassandra, l logr.Logger) error {
+	err := r.startSyncJob(c)
+	if err != nil {
+		l.Error(err, "Cannot start cluster status job",


status -> sync

Cannot start cluster synchronizer

ribaraka · 2024-02-05T11:45:20Z

controllers/clusters/cassandra_controller.go

+			c.Status.FromInstAPI(instaModel)
 			err = r.Status().Patch(context.Background(), c, patch)
 			if err != nil {
 				return err
 			}

-			if !areDCsEqual {
+			if !c.Status.DCEquals(iCassandra.Status.DataCentres) {


move c.Status.DCEquals(iCassandra.Status.DataCentres) before c.Status.FromInstAPI(instaModel)

ribaraka · 2024-02-05T15:13:47Z

controllers/clusters/cassandra_controller.go

+	return instaModel, nil
+}
+
+func (r *CassandraReconciler) applyDebeziumDependenciesIfNecessary(c *v1beta1.Cassandra, spec *models.CassandraCluster) error {


mergeDebezium

ribaraka · 2024-02-05T15:14:51Z

controllers/clusters/cassandra_controller.go

+func (r *CassandraReconciler) startSyncJob(c *v1beta1.Cassandra) error {
 	job := r.newWatchStatusJob(c)

 	err := r.Scheduler.ScheduleJob(c.GetJobID(scheduler.StatusChecker), scheduler.ClusterStatusInterval, job)


StatusChecker -> Synchronizer

I think we should change it after refactor of the all cluster resources

reminder: #661

ribaraka · 2024-02-05T15:16:57Z

controllers/clusters/cassandra_controller.go

+func (r *CassandraReconciler) startClusterJobs(c *v1beta1.Cassandra, l logr.Logger) error {
+	err := r.startSyncJob(c)
+	if err != nil {
+		l.Error(err, "Cannot start cluster status job",


Cannot start cluster synchronizer

ribaraka · 2024-02-05T15:20:29Z

controllers/clusters/cassandra_controller.go

 			err = r.Status().Patch(context.Background(), c, patch)
 			if err != nil {
 				return err
 			}

-			if !areDCsEqual {
+			if !dcsDiffer {


it should be differ, like: if dcsDiffer -> then do smth.
or change var name to dcsEqual.

worryg0d added the refactor Code improvements or refactorings label Feb 2, 2024

worryg0d requested a review from testisnullus February 2, 2024 14:33

worryg0d self-assigned this Feb 2, 2024

worryg0d requested a review from ribaraka February 2, 2024 14:33

worryg0d force-pushed the cassandra-codebase-refactor branch from c2ad0bd to 32eb62a Compare February 2, 2024 14:36

DoodgeMatvey requested changes Feb 2, 2024

View reviewed changes

worryg0d force-pushed the cassandra-codebase-refactor branch 2 times, most recently from fcddd6b to 1133a55 Compare February 5, 2024 09:43

worryg0d requested a review from DoodgeMatvey February 5, 2024 09:55

DoodgeMatvey requested changes Feb 5, 2024

View reviewed changes

worryg0d force-pushed the cassandra-codebase-refactor branch from 1133a55 to 6f4db10 Compare February 5, 2024 10:51

worryg0d requested a review from DoodgeMatvey February 5, 2024 10:51

DoodgeMatvey approved these changes Feb 5, 2024

View reviewed changes

ribaraka requested changes Feb 5, 2024

View reviewed changes

worryg0d force-pushed the cassandra-codebase-refactor branch from 6f4db10 to f4e65c3 Compare February 5, 2024 13:46

worryg0d requested a review from ribaraka February 5, 2024 13:46

worryg0d force-pushed the cassandra-codebase-refactor branch from f4e65c3 to 1cdc862 Compare February 5, 2024 13:52

ribaraka requested changes Feb 5, 2024

View reviewed changes

worryg0d force-pushed the cassandra-codebase-refactor branch from 1cdc862 to 113d526 Compare February 6, 2024 11:08

ribaraka approved these changes Feb 6, 2024

View reviewed changes

testisnullus approved these changes Feb 6, 2024

View reviewed changes

cassandra code base was refactored

2d7a827

worryg0d force-pushed the cassandra-codebase-refactor branch from 113d526 to 2d7a827 Compare February 6, 2024 13:34

testisnullus merged commit 055fe4f into main Feb 6, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cassandra codebase refactor #697

Cassandra codebase refactor #697

worryg0d commented Feb 2, 2024

DoodgeMatvey Feb 2, 2024

worryg0d Feb 5, 2024

DoodgeMatvey Feb 2, 2024

worryg0d Feb 5, 2024

DoodgeMatvey Feb 5, 2024

DoodgeMatvey Feb 2, 2024

worryg0d Feb 5, 2024

DoodgeMatvey Feb 2, 2024

worryg0d Feb 5, 2024

DoodgeMatvey Feb 2, 2024

worryg0d Feb 5, 2024

DoodgeMatvey Feb 5, 2024

worryg0d Feb 5, 2024

ribaraka Feb 5, 2024

ribaraka Feb 5, 2024

ribaraka Feb 5, 2024

ribaraka Feb 5, 2024

ribaraka Feb 5, 2024

ribaraka Feb 5, 2024

ribaraka Feb 5, 2024

ribaraka Feb 5, 2024

worryg0d Feb 6, 2024

ribaraka Feb 5, 2024

worryg0d Feb 6, 2024

ribaraka Feb 6, 2024

ribaraka Feb 5, 2024

ribaraka Feb 5, 2024

worryg0d Feb 6, 2024

Cassandra codebase refactor #697

Cassandra codebase refactor #697

Conversation

worryg0d commented Feb 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment