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

feat(mysql): support max_execution_time optimizer hint #15341

Merged
merged 21 commits into from
May 4, 2023

Conversation

SeongJaeSong
Copy link
Contributor

Pull Request Checklist

  • Have you added new tests to prevent regressions?
  • If a documentation update is necessary, have you opened a PR to the documentation repository?
  • Did you update the typescript typings accordingly (if applicable)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Does the name of your PR follow our conventions?

Relates issue

#15173

fzn0x
fzn0x previously approved these changes Nov 27, 2022
@fzn0x fzn0x requested a review from ephys November 27, 2022 10:48
src/model.d.ts Outdated Show resolved Hide resolved
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to add a new feature to sequelize

I would call the option maxExecutionTimeHintMs instead of maxExecutionTimeMs. It's likely that we'll add a feature that enforces a max execution time that works in all dialects and I don't know yet how compatible it would be with the hint from mysql.

src/dialects/abstract/query-generator.js Outdated Show resolved Hide resolved
src/dialects/abstract/query-generator.js Outdated Show resolved Hide resolved
src/dialects/abstract/query-generator.js Outdated Show resolved Hide resolved
src/dialects/mysql/index.ts Outdated Show resolved Hide resolved
test/unit/dialects/mysql/query-generator.test.js Outdated Show resolved Hide resolved
@SeongJaeSong SeongJaeSong requested review from ephys and fzn0x and removed request for ephys and fzn0x November 27, 2022 13:57
@SeongJaeSong SeongJaeSong marked this pull request as draft December 3, 2022 09:06
@SeongJaeSong SeongJaeSong marked this pull request as ready for review December 3, 2022 09:06
@SeongJaeSong SeongJaeSong requested review from WikiRik and ephys and removed request for ephys and WikiRik December 3, 2022 09:12
src/model.d.ts Outdated Show resolved Hide resolved
test/unit/query-generator/select-query.test.ts Outdated Show resolved Hide resolved
Co-authored-by: Rik Smale <13023439+WikiRik@users.noreply.github.com>
ephys
ephys previously requested changes Dec 10, 2022
Copy link
Member

@ephys ephys left a comment

Choose a reason for hiding this comment

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

Thanks for your patience

src/dialects/abstract/query-generator.js Outdated Show resolved Hide resolved
src/dialects/abstract/query-generator.js Outdated Show resolved Hide resolved
@SeongJaeSong SeongJaeSong requested review from WikiRik and ephys and removed request for ephys and WikiRik December 22, 2022 01:39
@SeongJaeSong
Copy link
Contributor Author

@ephys Is there a problem that we can't merge this?

@WikiRik
Copy link
Member

WikiRik commented Jan 2, 2023

Can you resolve the merge conflicts?

WikiRik
WikiRik previously approved these changes Jan 3, 2023
Copy link
Member

@WikiRik WikiRik left a comment

Choose a reason for hiding this comment

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

Looks good to me. I'll give @ephys some time to look at it as well

@SeongJaeSong
Copy link
Contributor Author

@WikiRik Is there any problem with this PR? If there is no plan to merge, I will close it.

@WikiRik
Copy link
Member

WikiRik commented May 3, 2023

@SeongJaeSong thanks for the reminder! If you can resolve the merge conflicts I think we can merge this in

@SeongJaeSong SeongJaeSong requested a review from WikiRik May 4, 2023 06:24
@WikiRik WikiRik dismissed ephys’s stale review May 4, 2023 06:56

Feedback incorporated

@WikiRik WikiRik merged commit fc3d6aa into sequelize:main May 4, 2023
47 checks passed
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.

None yet

4 participants