Show HN: GoWork – A Go Distributed Work Library | Hacker News Reader

Show HN: GoWork – A Go Distributed Work Library

github.com

68 pointsryanskidmore25 comments

To make the code simpler to read, I would suggest removing a bunch of "else {", like the ones on lines 84, 112 and 119 of main.go. When you have something like:

if err != nil { return err }

you don't need to put a else after the closing }.

Seriously? That's the critique you have?

Choices like that are 100% personal preference, and belief of which is "better" again is personal opinion.

Foe the counter argument; I find it far more readable to follow the if/else blocks, than to rely on identifying which if blocks contain return statements.

Personally, I prefer a step further, keeping all if/else blocks and using a variable inside the method to track the return value, with a single return statement at the end of the method.

There are plenty of blogs and articles debating for or against every permutation of this. But in the end, it really doesn't matter one bit whether there's an else statement on line X Y and Z.

It's not a critique but a suggestion. Like you are saying, it is not an error, my suggestion is simply to follow the standard library.

From golang.org Effective Go: "In the Go libraries, you'll find that when an if statement doesn't flow into the next statement—that is, the body ends in break, continue, goto, or return—the unnecessary else is omitted."

One reason you're getting downvoted is that the note the parent commenter gave is pretty squarely in Go idiom. Go favors early-exit, and the Go community is unusually strict about idiom.

(I'm pretty well acquainted with this particular issue because my C coding style is strictly anti-early-exit, which drives almost everyone I work with up a wall. I had to unlearn that habit to write idiomatic Go.)

Understandable, thanks for an explanation

I notice you're using interfaces to do your handlers, and (forgive me if i'm reading this wrong) casting those interfaces to a specific "type" of function before invoking them.

That seems like it would be inefficient in performance-critical code like a worker system. Have you considered using typed-functions and function pointers to accomplish a similar approach?

That said, I could easily be missing something in the design, so feel free to correct me if I'm wrong.

I've resolved this now, thanks :)

> Error is only returned by this function when the secret isn't 32 characters so as long as your secret is definitely 32 characters you can safely ignore the err returned by this function.

Personal preference, but in this case I'd advise using panic() instead of error. Ignoring errors is rarely a good idea: APIs change, other programmers read their code and don't know about your API, wonder why they ignored the error, &c. You can now also not change your API to ever actually return meaningful errors from that function anymore.

Meanwhile, if the failure mode is so well-defined, just put that in the documentation: "panics if the secret key is not exactly 32 characters long." Let the user of your library worry about it---they will have to, anyway: if they don't, they get an error they have to deal with, and suddenly they're dealing with it after all.

EDIT: From a quick glance, it looks like master gets results back from workers using .Get(<workid>)? Is it easily possible for master to "select" a bunch of workers and get the first completed one? If you use (or at least somehow allow the use of) channels for this, you get all the built-in Go goodness for this kind of control. Even for dynamic number of channels, the reflect package has a dynamic .Select that handles it for you. I.e.: provide a channel that I can <- work results from, and let Go (and the user of your lib) handle the rest. Including timeouts :) select { result, ok := <- workchan: ... ; <-time.NewTimer(3 * time.Seconds).C : <timeout> }.

I strongly disagree.

The package author means the error will be nil when the secret is 32 characters long; he doesn't mean you can completely ignore checking it.

Regarding when to use panic: "The convention in the Go libraries is that even when a package uses panic internally, its external API still presents explicit error return values." http://blog.golang.org/defer-panic-and-recover

Panics should only be used in cases of programmer error or when the process is in an unrecoverable state.

> The package author means the error will be nil when the secret is 32 characters long; he doesn't mean you can completely ignore checking it.

But that's exactly what will happen. This comment will lead to ws, _ := gowork.NewServer("32 character secret"). Which is a worse situation than a panic(). I understand the normal rule for this, but "a foolish consistency..". In this case, the init value being 32 long can be compared to "the init value must be non-nil." A lot of non-error APIs will panic() if you pass nil, even in the stdlib.

Doing panic() here will lead to less room for error and confusion, in my opinion. Especially because the error is explicit and loud (panic()), it's worth considering breaking the rules for.

If a developer is frequently ignoring returned errors, then they have larger problems and it's their fault. Panic was not added to assist lazy programmers. Even though some API has solidified, an author could later add different return errors. Heck, if that function was calling other functions with returned errors, then the list of possible unique errors increases greatly. You should never ignore an err because you think you know the possible reasons.

The package author should modify the phrasing of that comment.

Thanks for the feedback (both of you!) - when writing this, I envisioned a use case where the secret was static within the application, so it is easy enough for the developer to check their secret is 32 characters and not have to bother with checking that particular error. I didn't use panic because although a non 32 character secret puts my library in an unrecoverable state, it doesn't mean the application implementing the library is also in an unrecoverable state, hence why it returns an error instead of panicing.

Also, in terms of the top comment in this chain, the work server (master) never gets work from the workers, the workers push completed work back to the server, maybe this isn't clear within the current docs. All communication between the worker and server has to be coordinated by the application implementing the library.

An idea is to include a function in the library called MustNewServer() that panics instead of returning an error. A usability perk of having single return is that the function can be chained, i. e.,

    ws := gowork.MustNewServer(key).AddParams(...)

Great suggestion---I recant my original comment and second this.

Thanks for the suggestion, i'll add something similar to this when i've got a bit of time. Cheers!

As much as I hate chaining functions, I do like the idiom of declaring that a function may panic with the 'Must' prefix.

As long as there is an alternative function which returns errors :)

This looks nice enough as it is but what I think Go needs now is a more direct OTP clone suitable for new projects. If GoWork grew into that it could very well eat into Erlang's market share. Many people want their distributed systems language statically typed and fast. With Erlang you have the prospect of having to rewrite parts of your applications in C for speed looming menacingly on the horizon at all times. (Edit: I rewrote the last sentence to be more clear.)

A disclaimer may be appropriate here: I am aware of the limitations preventing one from reimplementing the entire OTP in Go. I still think that what you could get would be "close enough" with a few code guidelines (or a purpose-built static analyzer).

What do you mean by "OTP"? Do you mean "One Time Password", or is OTP an abbreviation for something else?

Erlang/OTP: http://www.erlang.org/faq/introduction.html

I meant Erlang/OTP.

I guess he means Open Telecom Platform which is a set of libraries of Erlang.

While it may appear that https://github.com/thejerf/reign is asleep, development is actually proceding apace; we're just not committing anything publicly because I'm of the opinion that nobody contributes to a project that doesn't at least have minimal functionality. We're currently on track to getting this put up in our beta environment pretty soon.

As the README.md says, I still wouldn't want this showing up on the HN mainlist, especially that repo as it stands now.

If there is someone who expresses a serious interest in helping out with that, I can see about doing a sync up to github. (If you're not comfortable discussing on HN, see my profile for contact info.)

(I also don't want to overpromise as I haven't looked into the details recently, but we are also working on functioning as an Erlang node sufficient to send and receive messages between Erlang and Go. We realized that we weren't willing to do a "big bang" upgrade, so we're planning on having cross-talk between the Erlang and Go nodes so we can gradually transition. I expect this to appeal to many people.)

Looks good but I always get reminded of this: https://twitter.com/rob_pike/status/618357610287226880

I say keep doing it, because it's a good heuristic for enthusiasm, lines of code, and perhaps quality.

As a little side node I'd personally rename main.go to core.go. Plus, I think there's room for splitting main.go into more little pieces to ease maintainability.

Reply on news.ycombinator.com