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: handle multiple http servers with one graceful service #944

Merged
merged 8 commits into from
Aug 13, 2024

Conversation

sc-zenokerr
Copy link
Contributor

@sc-zenokerr sc-zenokerr commented Aug 1, 2024

Prerequisite to fix sand issue: Scalingo/sand#271 (since graceful v1.1.2)
Related to PR #909
Related to Story STORY-828

  • Add a changelog entry in CHANGELOG.md

Copy link
Contributor

@leo-scalingo leo-scalingo left a comment

Choose a reason for hiding this comment

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

A small comment:

Do we really have to define the number of servers in the constructor as option. Can't it be counted when s.ListenAndServe is called?

@sc-zenokerr
Copy link
Contributor Author

We need to know "when" to call the tableflip upgrader "Ready()" method.

I saw three options:

  1. Add a method to the API to tell graceful that all listeners have been added
  2. Add a config option to let graceful know in advance how many httpServers to expect
  3. Add a short timeout so that all httpServers must be registered before the timeout

I chose option 2 because:

  • minimal change to the API
  • it is quite clear

I am happy to change it if you have any better ideas / preferences.

In the meantime, there is a "race" in this PR, so I am going to fix that, and send an update shortly.

@leo-scalingo
Copy link
Contributor

We need to know "when" to call the tableflip upgrader "Ready()" method.

I saw three options:

1. Add a method to the API to tell graceful that all listeners have been added

2. Add a config option to let graceful know in advance how many httpServers to expect

3. Add a short timeout so that all httpServers must be registered before the timeout

I chose option 2 because:

* minimal change to the API

* it is quite clear

I am happy to change it if you have any better ideas / preferences.

In the meantime, there is a "race" in this PR, so I am going to fix that, and send an update shortly.

Really good explanation, LGTM

Copy link
Contributor

@leo-scalingo leo-scalingo left a comment

Choose a reason for hiding this comment

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

Validating the chosen methodology, letting Pierre for the code analysis :)

sc-zenokerr added a commit to Scalingo/sand that referenced this pull request Aug 1, 2024
sc-zenokerr added a commit to Scalingo/sand that referenced this pull request Aug 1, 2024
Related to Scalingo/go-utils#944

Finalise merge of master
sc-zenokerr added a commit to Scalingo/sand that referenced this pull request Aug 1, 2024
sc-zenokerr added a commit to Scalingo/sand that referenced this pull request Aug 1, 2024
@sc-zenokerr
Copy link
Contributor Author

@curzolapierre this is ready for code review now.

Copy link
Member

@curzolapierre curzolapierre left a comment

Choose a reason for hiding this comment

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

Just some comments about style of logs and errors.

Otherwise LGTM

graceful/service.go Outdated Show resolved Hide resolved
graceful/service.go Outdated Show resolved Hide resolved
graceful/service.go Outdated Show resolved Hide resolved
graceful/service.go Outdated Show resolved Hide resolved
graceful/service.go Outdated Show resolved Hide resolved
graceful/service.go Outdated Show resolved Hide resolved
graceful/service.go Show resolved Hide resolved
graceful/service.go Outdated Show resolved Hide resolved
graceful/service.go Outdated Show resolved Hide resolved
@sc-zenokerr
Copy link
Contributor Author

@EtienneM Before I merge this, I would appreciate a discussion about it if you have time. Thanks in advance

sc-zenokerr added a commit to Scalingo/sand that referenced this pull request Aug 13, 2024
Related to Scalingo/go-utils#944
With etienne's expert eye
@sc-zenokerr sc-zenokerr merged commit 1dbaa4a into master Aug 13, 2024
2 checks passed
@sc-zenokerr sc-zenokerr deleted the feat/allow-multiple-listeners branch August 13, 2024 16:16
sc-zenokerr added a commit to Scalingo/sand that referenced this pull request Aug 13, 2024
Related to Scalingo/go-utils#944
Fix TLS config initialisation
sc-zenokerr added a commit to Scalingo/sand that referenced this pull request Aug 14, 2024
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.

3 participants