Skip to content
This repository was archived by the owner on May 26, 2023. It is now read-only.

Improve stability of workflows ALL viev #315

Merged
merged 1 commit into from
May 3, 2021

Conversation

feedmeapples
Copy link
Contributor

@feedmeapples feedmeapples commented Apr 30, 2021

What was changed:

Workflows page: takes a different approach on how ALL view works.

Before:
Per one page of workflows, it was fetching both Open and Closed workflows and then making additional fetch, either open or closed, to account for the startTime time difference

Now:
Per one page of workflows, it fetches ONLY Open or Closed workflows, depending on which one is behind in startTime.
This results in much better predictability and stability, as in this approach we don't have to manually change the time range of workflows to fetch for syncing reasons. When scrolling and fetching more pages, the ordering is maintained because we only fetch the type of workflows that is behind

Why?

Improves stability of ALL view. Before there were few corner case that would break fetching mechanism

Checklist

  1. Closes issue:

  2. How was this tested:

Mostly manually to verify the ordering of Open/Closed fetches

To create sample data, you can take the helloworld example from here and do the below modifications https://github.com/temporalio/samples-go/tree/master/helloworld

Executing starter/main.go will create 20 workflows:

  1. keep argument makes the workflow remain Open for 10 minutes if set to true, or otherwise the workflows will be quickly Close.
  2. Change the Namespace: "default" to another namespace for testing with a new set of data/runs

Testing:

  1. Run the below code with the keep flag set to true
    expected: on Web UI ALL workflows view you
  2. Open the ALL view in Web UI, the workflows ordering should ~closely resemble the ordering that you have used.
  3. Scroll down the pages
    expected: the fetching stops as the oldest workflows are fetched
  4. Change the ALL view to Open/Closed and back to ALL
    expected: ALL view starts starts all over without issues

Modified helloworld sample code:

hellloworld/starter/main.go :

func main() {
	// The client is a heavyweight object that should be created once per process.
	c, err := client.NewClient(client.Options{Namespace: "default"})
	if err != nil {
		log.Fatalln("Unable to create client", err)
	}
	defer c.Close()

	uuidvar := uuid.New()
	i := 1
	for i <= 20 {
		id := uuidvar[:8] + "###___" + strconv.Itoa(i)
		i++

		workflowOptions := client.StartWorkflowOptions{
			ID:        id,
			TaskQueue: "hello-world",
		}

		_, err := c.ExecuteWorkflow(context.Background(), workflowOptions, helloworld.Workflow,
			"Temporal", true)
		if err != nil {
			log.Fatalln("Unable to execute workflow", err)
		}
	}
}

helloworld.go :

// Workflow is a Hello World workflow definition.
func Workflow(ctx workflow.Context, name string, keep bool) (string, error) {
	ao := workflow.ActivityOptions{
		StartToCloseTimeout: 10 * time.Second,
	}
	ctx = workflow.WithActivityOptions(ctx, ao)

	logger := workflow.GetLogger(ctx)
	logger.Info("HelloWorld workflow started", "name", name)

	var result string
	err := workflow.ExecuteActivity(ctx, Activity, name, keep).Get(ctx, &result)
	if err != nil {
		logger.Error("Activity failed.", "Error", err)
		return "", err
	}

	logger.Info("HelloWorld workflow completed.", "result", result)

	return result, nil
}

func Activity(ctx context.Context, name string, keep bool) (string, error) {
	logger := activity.GetLogger(ctx)
	logger.Info("Activity", "name", name)
	if keep {
		time.Sleep(10 * time.Minute)
	}
	return "Hello " + name + "!", nil
}
  1. Any docs updates needed?

No

@feedmeapples feedmeapples linked an issue Apr 30, 2021 that may be closed by this pull request
@feedmeapples
Copy link
Contributor Author

feedmeapples commented Apr 30, 2021

@just-at-uber this takes a bit different approach on keeping Open and Closed workflows in ALL view ~ordered by startTime

i've checked the code in cadence-web, it seems in the ALL view there can be unordered completely. Feel free to pull these changes if you find them useful

@feedmeapples feedmeapples requested a review from swyxio April 30, 2021 05:36
Copy link
Contributor

@swyxio swyxio left a comment

Choose a reason for hiding this comment

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

i left some comments in our slack but this is fine if need to merge fast

@feedmeapples feedmeapples merged commit 4e6b06c into master May 3, 2021
@feedmeapples feedmeapples deleted the improve-all-stability branch May 3, 2021 19:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dashboard query workflow list repeatly on first page
2 participants