✨ Introducing examples module #14

PeterEFinch · 2022-03-04T00:49:14Z

No description provided.

ghost

Use examples_test.go & Example…(). Then you (and I) can be sure that this example will work now and in future :-)

ghost · 2022-03-07T18:01:29Z

examples.md

+		// The specifics of gracefully shutting down the service, include the timeout
+		// and error handling, should be decided by the user.


Kinda. There’s a hard kill-timeout on a pod, though. Think it’s 30sec. by default.

This is a general library for managing shutdowns. I want to avoid tying it to certain technologies, e.g. GCloud/Kubernetes, as this would limit its usage. Additionally, if there are multiple shutdown actions it is then amount of time given for each might different.

Yeah, fine. However, I think it would be nice to mention the GKE-specifics in the comments. As this is a company-only library (yet).

I am still reluctant as my goal is to not tie this to a technology even in a comment.

my goal is to not tie this to a technology even in a comment.

I don’t get it, really:

What’s your intended audience? As this library (currently) resides only internally in our small company, the technology-setup is clear: Kubernetes with containerd/OCI-runtime, i.e. somehow POSIX-compatibility. Therefore I think it makes sense to give some explanation for it. At least in a comment.

The library is tied to the technology-setup above anyway. E.g. a (rather) quick search shows that it would not work even on Windows (see Unable to react to graceful shutdown of (Windows) container moby/moby#25982). To be fair, (only?) part of if fixed quite recently (2020) in Go, though.

The intended audience is the developers at Graphmasters. My approach is more about creating minimal code and independent code (with comments).

“minimal”: agreed and like
“independent”: unrealistic dream (see above)

Actually, independence for a developer in a DevOps-culture means (s)he needs to have, at least a basic, understanding of the underlying technology stack (what historically only operators had). I.e. it would help giving some insights on the underlying technology here to raise independence of the developers ☝🏿

See e.g. this K8s-failure-story (https://k8s.af): Kubernetes' dirty endpoint secret and Ingress — Grace is overrated. That’s a topic we still see on our services, i.e. there’s no good solution found. Means our (Go-)code depends on the infrastructure/technology used.

examples.md

ghost · 2022-03-07T18:03:43Z

examples.md

+	// The server starts listening. It should never encounter an error other
+	// than the server being closed.
+	if err := server.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
+		log.Fatalf("server encountered error: %v", err)


Is closing immediately the right thing to do here when using this module? I would say no. It’s contradictory to what safe down means.

Safedown allows you the ability to coordinate shutting down across multiple go routines and gracefully in the face of an interrupt (or similar) signal.

What exactly should happen here is unclear and probably a debate for best practices (the primary purpose of this library is to provide functionality not demonstrate best practices).

In this case the graceful shutdown is primarily for the HTTP server. Arguably, if ListenAndServe fails then there is no need to be graceful.

If you can say what you think should happen here and why then I am willing to change it.

I would say an error-log would be the correct thing. You assume that there’s only one server running. But that’s not the case, e.g. most have an extra status-server where it would be really good to serve (error-)metrics esp. in case the main server(s) fail. Other examples are many gRPC-services (one server to public, one cluster-internal).

@DominikGM This is a best practices issue. I have seen people log the errors, have fatals and have panics. I know what I would use, I just don't want this place to be a best practices and have to change stuff because what is considered best has changed.

Ok. Then the only reasonable thing you could do is to use a comment like // handle error
Code of a module is often consumed as a best-practice thing. You’re right that it shouldn’t be done like this, but that’s my experience from reality.

Therefore

I have seen people […] have fatals and have panics.

is still wrong. As you have a Fatal() in it, this implicit best-practice isn’t good practice, IMO.

@DominikGM I still don't think there is a definitive right way and think that using a fatal is a reasonable choice. Nevertheless, I have changed it to just a log and added a comment because if it had been this way at the start I would be fine with it.

if err := server.ListenAndServe(); !errors.Is(err, http.ErrServerClosed) { // It is up to the user how the error should be handled. This is only // for illustrative purposes. log.Printf("server encountered error: %v", err) }

With using Fatal(), this would result in a short 502-spike (perhaps for <= 1% calls). Still if you have a sensitive alert on this, you would notice this. See the example mentioned above: #14 (comment)

examples.md

PeterEFinch · 2022-03-07T22:21:02Z

Use examples_test.go & Example…(). Then you (and I) can be sure that this example will work now and in future :-)

@DominikGM I specifically didn't use examples_test.go because:

I don't want ANY dependencies in this library.
Examples are run along side tests and since this main function should never stop the example would cause problems with testing unless we added additional code to send an interrupt signal, which I think would complicate the code.

There is certainly a benefit to writing example tests (and there is already one), however, I think the merits aren't worth it in this situation.

PeterEFinch · 2022-03-15T01:31:54Z

@DominikGM Just a ping for a week of inactivity.

PeterEFinch · 2022-03-17T23:08:00Z

@DominikGM In my opinion the best thing about this library is that it is simple, it does one thing, and it is independent of pretty much everything. I have tried to get it to a stage where it doesn't need to be touched or actively maintained. It is for this reason that I am opposed to anything which changes this. I know it is a strict convention. It does seem to be working so far.

PeterEFinch · 2022-03-22T00:48:38Z

This pull request addresses the concern of having a practical example that represents a service, specifically a HTTP service.

I think it is getting to the stage where the benefits of any changes is less than the effort of discussing it. As such, I propose that, unless there are major or significant issues, we move on from this pull request.

If there is additional feedback received that examples are lacking, confusing or lead developers to misunderstandings then we can address it at a future date.

PeterEFinch · 2022-05-06T04:49:05Z

@DominikGM Instead of having the code in a readme, there is now another module just containing examples.

I intend to change the circleci to github actions soon at which stage I will also add a step for building the mains in the examples module. This way we can be sure that the code in the examples module will always build.

PeterEFinch · 2022-05-07T01:20:26Z

I am making the decision as admin and lead maintainer of this library that this pull request is suitable for merging.

PeterEFinch added 6 commits March 4, 2022 10:29

📝 Writing common examples doc.

ffe81a5

📝 Writing common examples doc.

8e6c1fc

📝 Linking to common examples

a04ab17

🚚 Renaming md file

08ed1b4

📝 Writing common examples doc.

c3ba967

📝 Writing common examples doc.

0f4252b

PeterEFinch added the enhancement New feature or request label Mar 4, 2022

PeterEFinch requested a review from a user March 4, 2022 00:49

PeterEFinch mentioned this pull request Mar 4, 2022

Example: Why cancelling a context on shutdown? #13

Open

✏️ Fixing typos.

7fa51e2

ghost suggested changes Mar 7, 2022

View reviewed changes

ghost reviewed Mar 7, 2022

View reviewed changes

📝 Updating examples doc

8414291

ghost mentioned this pull request Mar 16, 2022

Use example_test.go #15

Closed

ghost mentioned this pull request Mar 18, 2022

✅ Reworking tests and new examples #16

Merged

PeterEFinch added 2 commits March 19, 2022 09:07

📝 Updating examples docs.

9cc5119

📝 Updating examples docs.

559a661

PeterEFinch added 3 commits May 6, 2022 14:37

🔧 Creating go module

585c487

💥 Making .md file into a .go file

1a2a892

📝 Minimise changes to readme.

2806b52

PeterEFinch changed the title ~~📝 Adding additional examples~~ ✨ Introducing examples module May 7, 2022

PeterEFinch merged commit 4554ae8 into master May 7, 2022

PeterEFinch deleted the documentation/more_examples branch May 7, 2022 01:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Introducing examples module #14

✨ Introducing examples module #14

		// The specifics of gracefully shutting down the service, include the timeout
		// and error handling, should be decided by the user.

✨ Introducing examples module #14

✨ Introducing examples module #14

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment