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

Update default connection heartbeat length #47

Merged

Conversation

jackgene
Copy link
Contributor

@jackgene jackgene commented Aug 21, 2022

Calculate default timeout_heartbeat_interval with minimum of 1s

This is as specified in the accompanying code comment "use a quarter of the actual configured timeout but at minimum 1 second".

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Aug 21, 2022
@SeanTAllen
Copy link
Member

Please remove the unrelated gitignore change.

@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Aug 21, 2022
@ponylang-main
Copy link
Contributor

Hi @jackgene,

The changelog - fixed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 47.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@SeanTAllen
Copy link
Member

Please update the first comment here to be the commit message to use when we merge.

First line should be a short sentence on what was fixed.

Body should have additional detail.

@jackgene
Copy link
Contributor Author

Please remove the unrelated gitignore change.

Oh yeah, accidentally included that.

@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Aug 24, 2022
@SeanTAllen
Copy link
Member

This shouldn't be merged until release notes are added.

@jackgene
Copy link
Contributor Author

This shouldn't be merged until release notes are added.

Will address this shortly, but I do have a question, per @ponylang-main:

We suggest you call the file 47.md to match the number of this pull request.

But looking at the files in currently in .release-notes, they all seem to be version numbers. Should I name the file 0.4.2.md (which seems like the natural next number)? Or should I update next-release.md and it'll automatically handle the rest? Or should I use 47.md per @ponylang-main's suggestion?

Thank you, and sorry about the delays on this one.

@rhagenson
Copy link
Member

rhagenson commented Aug 25, 2022

Follow what ponylang-main says and call it 47.md

47 is the PR number.

The others are release entries. When a release is done the non-release entries are rolled together into a new release entry.

--

Edit: not quite how it works, ponylang-main builds next-release.md in the middle there and then uses that for versioned releases.

@jackgene
Copy link
Contributor Author

Just added the release notes. Please let me know if you have any concerns with it.

@rhagenson
Copy link
Member

I just took a look and do not see any issue with the release notes you added. This fixed a bug where the minimum of 1 second was incorrectly set to a minimum of 1 millisecond. All of that is now in the release notes.

@SeanTAllen SeanTAllen changed the title Minimum default heartbeat 1000ms Update default connection heartbeat length Aug 26, 2022
@SeanTAllen
Copy link
Member

I've updated the PR title to better work in the CHANGELOG.

I've updated the release notes to be less "commit message" and more "release notes". Feel free to ask any questions about the changes so you can adapt any future titles etc as you make additional contributions.

@SeanTAllen SeanTAllen merged commit 60a715d into ponylang:main Aug 26, 2022
@SeanTAllen SeanTAllen removed do not merge This PR should not be merged at this time discuss during sync Should be discussed during an upcoming sync labels Aug 26, 2022
github-actions bot pushed a commit that referenced this pull request Aug 26, 2022
github-actions bot pushed a commit that referenced this pull request Aug 26, 2022
@jackgene jackgene deleted the bugfix/minimum_default_heartbeat_1000ms branch August 27, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants