Feature/state machine changes #454

franmoore05 · 2024-11-01T16:58:30Z

What

Draft PR for state machine implementation. Tests have not been updated yet.

How to review

Ensure the changes make sense.

Who can review

@janderson2

janderson2

Have added a few thoughts on the structure.

janderson2 · 2024-11-04T10:21:55Z

application/orchestrator.go

+	//nolint:gosec // This is not a hardcoded credential.
+	downloadServiceToken = "X-Download-Service-Token"


Would the following address the lint error without need for the comment? (haven't tested it, so just a theory)

Suggested change

//nolint:gosec // This is not a hardcoded credential.

downloadServiceToken = "X-Download-Service-Token"

//nolint:gosec // This is not a hardcoded credential.

downloadServiceTokenHeader = "X-Download-Service-Token"

I'll address this in the overall PR for this change.

application/orchestrator.go

application/states.go

application/state_machine.go

janderson2

Shaping up nicely. I think this will definitely improve readability and maintainability immensely. Have left some thoughts and comments.

service/service.go

application/state_machine.go

service/service.go

janderson2 · 2024-11-12T10:32:28Z

application/orchestrator.go

+	return nil
+}
+
+func castStateToState(state string) State {


It looks like this could be handled by the state machine itself as it has the map[string]State state map so can complete this cast internally. See comment on Transition function.

application/state_machine.go

application/orchestrator.go

janderson2 · 2024-11-12T11:09:22Z

application/orchestrator.go

+	if vars[hasDownloads] != trueStringified {
+		data["updated_state"] = versionUpdate.State
+		if versionUpdate.State == models.PublishedState {
+			if err := smDS.publishVersion(ctx, currentDataset, currentVersion, versionUpdate, versionDetails); err != nil {
+				log.Error(ctx, "putVersion endpoint: failed publishing version", err)
+				return err
+			}
+		}
+
+		if versionUpdate.State == models.AssociatedState && currentVersion.State != models.AssociatedState {
+
+			if err := smDS.associateVersion(ctx, currentVersion, versionUpdate, versionDetails); err != nil {
+				log.Error(ctx, "putVersion endpoint: failed associating version", err)
+				return err
+			}
+		}
+	}


This logic should form part of the transition into published and associated states respectively and should therefore be called from the Enter functions of those states.

Is the thinking is to complete the refactor in two steps to minimise the scope of refactoring in a single PR?

This has been moved in to the enter function, let me know if this looks right to you, if it is I'll implement for the associateVersion also.

One comment about the locking, but looks right to me.

application/states.go

janderson2 · 2024-11-15T09:27:16Z

application/state_machine.go

+			for i := 0; i < len(transitions); i++ {
+				if previousState == transitions[i] {
+					match = true
+					nextState = castStateToState(newState)


This call to the hard coded cast in the castStateToState() could be replaced with:

Suggested change

nextState = castStateToState(newState)

nextState = sm.states[newState]

This would improve the maintainability of the code by preventing the need to extend the hard coded cast when changing the states.

This has been amended

I am not seeing this change. Maybe it hasn't been pushed?

application/state_machine.go

application/states.go

janderson2 · 2024-11-19T15:56:11Z

application/application.go

+		fmt.Println("Validation Failed")
+		fmt.Println(errModel)
+		return errModel
+	} else {


else is redundant here as the above will have returned on error.

This has now been removed

janderson2 · 2024-11-19T16:05:32Z

application/application.go

+	lockID, error := smDS.DataStore.Backend.AcquireInstanceLock(ctx, currentVersion.ID)
+	if error != nil {
+		return error
+	}
+	defer func() {
+		smDS.DataStore.Backend.UnlockInstance(ctx, lockID)
+	}()


I think we may need to move the locking to the orchestrator outside of the enter function (e.g. call it in the the AmendVersion function) so that the lock is acquired before the current version data is read to prevent a race condition. This will also mean that the lock will be implemented across the different transitions.

It looks like we might need to consider a dataset level lock as well, but as I don't believe that currently exists it is a later concern.

I have now moved where the lock happens

janderson2 · 2024-11-19T16:11:22Z

application/application.go

+		}
+
+		// This is only applicable to CMD datasets
+		if currentVersion.Type == "v4" {


It appears that CMD could have multiple string values here. Suggest calling

dp-dataset-api/models/dataset.go

Lines 54 to 57 in d562530

func GetDatasetType(datasetType string) (DatasetType, error) {

switch datasetType {

case "filterable", "v4", "":

return Filterable, nil

and Filterable rather than a string literal to avoid any issues.

This check has now been amended

janderson2 · 2024-11-19T16:14:58Z

application/orchestrator.go

+	if vars[hasDownloads] != trueStringified {
+		data["updated_state"] = versionUpdate.State
+		if versionUpdate.State == models.PublishedState {
+			if err := smDS.publishVersion(ctx, currentDataset, currentVersion, versionUpdate, versionDetails); err != nil {
+				log.Error(ctx, "putVersion endpoint: failed publishing version", err)
+				return err
+			}
+		}
+
+		if versionUpdate.State == models.AssociatedState && currentVersion.State != models.AssociatedState {
+
+			if err := smDS.associateVersion(ctx, currentVersion, versionUpdate, versionDetails); err != nil {
+				log.Error(ctx, "putVersion endpoint: failed associating version", err)
+				return err
+			}
+		}
+	}


One comment about the locking, but looks right to me.

janderson2 · 2024-11-19T16:17:10Z

application/application.go

+	return nil
+}
+
+func UpdateVersionInfo(smDS *StateMachineDatasetAPI, ctx context.Context,


Consider whether this needs to be exported.

I've now amended this

janderson2 · 2024-11-19T16:29:03Z

application/application.go

+		if err := generator.Generate(ctx, versionDetails.datasetID, versionUpdate.ID, versionDetails.edition, versionDetails.version); err != nil {
+			data["instance_id"] = versionUpdate.ID
+			data["state"] = versionUpdate.State
+			data["type"] = t.String()
+			log.Error(ctx, "State Machine: Publish: PublishDataset: error while attempting to generate full dataset version downloads on version publish", err, data)
+			return err
+			// TODO - TECH DEBT - need to add an error event for this.  Kafka message perhaps.
+		}
+		log.Info(ctx, "State Machine: Publish: PublishDataset: generated full dataset version downloads:", data)


Need to check that generator is not nil. Something like:

Suggested change

if err := generator.Generate(ctx, versionDetails.datasetID, versionUpdate.ID, versionDetails.edition, versionDetails.version); err != nil {

data["instance_id"] = versionUpdate.ID

data["state"] = versionUpdate.State

data["type"] = t.String()

log.Error(ctx, "State Machine: Publish: PublishDataset: error while attempting to generate full dataset version downloads on version publish", err, data)

return err

// TODO - TECH DEBT - need to add an error event for this. Kafka message perhaps.

}

log.Info(ctx, "State Machine: Publish: PublishDataset: generated full dataset version downloads:", data)

if generator == nil {

log.Info(ctx, "PublishDataset: no download generation needed for dataset type", data)

} else {

if err := generator.Generate(ctx, versionDetails.datasetID, versionUpdate.ID, versionDetails.edition, versionDetails.version); err != nil {

data["instance_id"] = versionUpdate.ID

data["state"] = versionUpdate.State

data["type"] = t.String()

log.Error(ctx, "State Machine: Publish: PublishDataset: error while attempting to generate full dataset version downloads on version publish", err, data)

return err

// TODO - TECH DEBT - need to add an error event for this. Kafka message perhaps.

}

log.Info(ctx, "State Machine: Publish: PublishDataset: generated full dataset version downloads:", data)

}

The nil check has now been added

janderson2 · 2024-11-19T16:38:37Z

application/state_machine.go

+					nextState = castStateToState(versionUpdate.State)
+					if nextState == nil {
+						return errors.New("incorrect state value")
+					}


This function call can be simplified as something like:

Suggested change

nextState = castStateToState(versionUpdate.State)

if nextState == nil {

return errors.New("incorrect state value")

}

nextState, ok = castStateToState(versionUpdate.State)

if !ok {

return errors.New("incorrect state value")

}

Thereby removing the need for a hard coded cast function.

This has now been amended

janderson2 · 2024-11-20T16:43:36Z

application/application.go

+	// lockID, error := smDS.DataStore.Backend.AcquireInstanceLock(ctx, currentVersion.ID)
+	// if error != nil {
+	// 	return error
+	// }
+	// defer func() {
+	// 	smDS.DataStore.Backend.UnlockInstance(ctx, lockID)
+	// }()


A little tidy up needed

franmoore05 · 2024-12-30T17:22:11Z

Closing this PR - new one to follow once unit tests and component tests are fixed.

franmoore05 added 5 commits October 23, 2024 12:02

merge conflicts

b7588cc

stashed changes

56927db

Initial code for state machine

7955850

Further changes

59a2b04

tidy up

efc0c26

janderson2 reviewed Nov 4, 2024

View reviewed changes

franmoore05 added 2 commits November 7, 2024 17:23

Updates following PR

e443b5a

Removal of test code

0aa1479

janderson2 reviewed Nov 12, 2024

View reviewed changes

franmoore05 added 2 commits November 12, 2024 19:37

Further PR updates

b875df1

Updates following PR

e7f6175

janderson2 reviewed Nov 15, 2024

View reviewed changes

More changes following PR

4fa976e

janderson2 reviewed Nov 19, 2024

View reviewed changes

Updates following PR

dc0896f

janderson2 reviewed Nov 20, 2024

View reviewed changes

franmoore05 closed this Dec 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/state machine changes #454

Feature/state machine changes #454

franmoore05 commented Nov 1, 2024

janderson2 left a comment

janderson2 Nov 4, 2024

franmoore05 Nov 7, 2024

janderson2 left a comment

janderson2 Nov 12, 2024

janderson2 Nov 12, 2024

franmoore05 Nov 13, 2024

janderson2 Nov 19, 2024

janderson2 Nov 15, 2024

franmoore05 Nov 19, 2024

janderson2 Nov 19, 2024

janderson2 Nov 19, 2024

franmoore05 Nov 20, 2024

janderson2 Nov 19, 2024

franmoore05 Nov 20, 2024

janderson2 Nov 19, 2024 •

edited

Loading

franmoore05 Nov 20, 2024

janderson2 Nov 19, 2024

janderson2 Nov 19, 2024

franmoore05 Nov 20, 2024

janderson2 Nov 19, 2024

franmoore05 Nov 20, 2024

janderson2 Nov 19, 2024

franmoore05 Nov 20, 2024

janderson2 Nov 20, 2024

franmoore05 commented Dec 30, 2024

		//nolint:gosec // This is not a hardcoded credential.
		downloadServiceToken = "X-Download-Service-Token"

	nextState = castStateToState(newState)
	nextState = sm.states[newState]

	func GetDatasetType(datasetType string) (DatasetType, error) {
	switch datasetType {
	case "filterable", "v4", "":
	return Filterable, nil

Feature/state machine changes #454

Feature/state machine changes #454

Conversation

franmoore05 commented Nov 1, 2024

What

How to review

Who can review

janderson2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janderson2 left a 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

janderson2 Nov 19, 2024 • edited Loading

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

franmoore05 commented Dec 30, 2024

janderson2 Nov 19, 2024 •

edited

Loading