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.

Sign up for GitHub

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

Jump to bottom

fix(api): Synchronise schema publishing per target #764

Merged
merged 15 commits into from
Dec 15, 2022
Merged

fix(api): Synchronise schema publishing per target #764

merged 15 commits into from
Dec 15, 2022

Conversation

enisdenjo
Copy link
Member

@enisdenjo enisdenjo commented Dec 7, 2022 •
edited by n1ru4l
Loading

Closes #728
Closes #756

@changeset-bot
Copy link

changeset-bot bot commented Dec 7, 2022 •
edited
Loading

⚠️ No Changeset found

Latest commit: b3c7d7a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Dec 7, 2022 •
edited
Loading

🚀 Website Preview

The latest changes to the website are available as preview in: https://0fa1c308.hive-landing-page.pages.dev

@enisdenjo enisdenjo force-pushed the race branch 2 times, most recently from c3dea6c to e4f4e8d Compare December 8, 2022 10:58
@enisdenjo enisdenjo changed the title fix: Synchronise schema publishing per target fix(api): Synchronise schema publishing per target Dec 8, 2022
@enisdenjo enisdenjo marked this pull request as ready for review December 8, 2022 11:35
n1ru4l
n1ru4l approved these changes Dec 12, 2022
View reviewed changes
Copy link
Contributor

@n1ru4l n1ru4l left a comment

Choose a reason for hiding this comment

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

I wonder whether we should also add a test for publishing two targets in parallel.

Aside from that the logic makes sense to me!

packages/services/storage/src/index.ts Outdated
if (advisoryLock) {
lockConn.query(sql`select pg_advisory_unlock(${idInt})`).catch(err => {
// TODO: what to do instead?
console.error(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can use logger.warn for now - if we notice it keeps happening a lot we can think more about it.

@enisdenjo
Copy link
Member Author

@n1ru4l right, that test would make sense. I'll see what I can do. 👌

@kamilkisiela
Copy link
Contributor

@enisdenjo integration tests are failing :(

@enisdenjo
Copy link
Member Author

@kamilkisiela fixed, the issue was mainly caused by nodejs/node#38924.

@kamilkisiela kamilkisiela merged commit f6090f1 into main Dec 15, 2022
@kamilkisiela kamilkisiela deleted the race branch December 15, 2022 19:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@n1ru4l n1ru4l n1ru4l approved these changes

@kamilkisiela kamilkisiela kamilkisiela approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Race condition in the schema publishing flow
3 participants
@enisdenjo @kamilkisiela @n1ru4l