Skip to content

[feat-2433]: record execution time for plugin subtasks#2455

Merged
klesh merged 3 commits into
apache:mainfrom
merico-ai:issues/2433
Aug 3, 2022
Merged

[feat-2433]: record execution time for plugin subtasks#2455
klesh merged 3 commits into
apache:mainfrom
merico-ai:issues/2433

Conversation

@keon94
Copy link
Copy Markdown
Contributor

@keon94 keon94 commented Jul 8, 2022

⚠️   Pre Checklist

Please complete ALL items in this checklist, and remove before submitting

  • I have read through the Contributing Documentation & PR Template
  • This PR is using a label (bug, feature etc.)
  • My code is has necessary documentation (if appropriate)
  • I have added any relevant tests
  • This section (⚠️   Pre Checklist) will be removed when submitting PR

Summary

Does this close any open issues?

#2433

Screenshots

included below.

Other Information

Any other information that is important to this PR.

@keon94 keon94 changed the title feat(framework): add execution time for plugin subtasks [feat-2433] record execution time for plugin subtasks Jul 8, 2022
@keon94 keon94 changed the title [feat-2433] record execution time for plugin subtasks [feat-2433]: record execution time for plugin subtasks Jul 8, 2022
@keon94 keon94 force-pushed the issues/2433 branch 3 times, most recently from b369053 to bffe805 Compare July 12, 2022 00:32
Comment thread models/migrationscripts/subtask_07112022.go Outdated
@keon94 keon94 force-pushed the issues/2433 branch 3 times, most recently from 2efcd5a to 51bf84a Compare July 13, 2022 03:05
@keon94 keon94 marked this pull request as ready for review July 13, 2022 03:05
@keon94
Copy link
Copy Markdown
Contributor Author

keon94 commented Jul 13, 2022

Screenshot of the new subtasks table after a manual test

image

@keon94 keon94 requested a review from klesh July 13, 2022 03:07
"time"
)

// Subtask_07112022 DB snapshot model of models.Subtask
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@klesh is this still the correct way of managing migrations? I see recently there was a big refactor and everything is using init_schema.go now as opposed to individual migrations

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We added MultiConnection for all other plugins in v0.12. With this connection table added, every single table of the plugin was affected, as well as the Domain Layer tables, we have to flush the no-longer-valid data to maintain the data integrity, So, might as well drop existing migration scripts and rebuild the tables.
However, the Framework migration should be not affected, we should keep the migration scripts as it was, with that said, you are doing the right thing.

@keon94 keon94 force-pushed the issues/2433 branch 3 times, most recently from 3e31aa6 to b0b7f46 Compare July 20, 2022 19:18
Comment thread runner/run_task.go
@keon94 keon94 force-pushed the issues/2433 branch 2 times, most recently from 3a2cae2 to 3b61ec4 Compare August 2, 2022 03:36
@keon94 keon94 requested a review from klesh August 2, 2022 04:04
Copy link
Copy Markdown
Contributor

@klesh klesh left a comment

Choose a reason for hiding this comment

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

LGTM

@klesh klesh merged commit f8f3cde into apache:main Aug 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants