<p>Hey all. I'm a hobbyist programmer at best and getting back into programming I decided to tackle Go! I am having a blast with it so far and I'm currently working on sort of a managed job queue. Where you can set up jobs, send them to the queue, and the queue will fire up a MaxActiveJobs number of jobs and wait for them to finish and continue on.</p>
<p>The goofy example code I wrote doesn't actually do any processing, it just sleeps for 10 seconds, acting like it was doing work... just to showcase what ive got working so far.</p>
<p>So yeah, any comments, critique, advice would be much appreciated! Like I said i'm just getting back into programming so if you see any bad habits, I'd like to know about them sooner than later.</p>
<p>The library: <a href="https://github.com/thegtproject/jobqueue/tree/channel" rel="nofollow">https://github.com/thegtproject/jobqueue/tree/channel</a></p>
<p>(It is far from complete, but I think i have enough to show as it is now that people can see where im going with it/my thought process)</p>
<p>Its also my first time taking a stab at interfaces in go!</p>
<hr/>**评论：**<br/><br/>abiosoft: <pre><p>Always put basic info and usage of the library in the README. The extra repo for the example is not needed yet, it could've been a directory in the repo.</p>
<p>Since this is a library, documentation matters even more. Add <a href="https://godoc.org/github.com/thegtproject/jobqueue" rel="nofollow">https://godoc.org/github.com/thegtproject/jobqueue</a> link to the readme and ensure you document all public types, functions and methods.</p>
<p>I can see you are spawning additional goroutine. You should probably add test and run your tests with the <code>-race</code> flag to ensure there are no data races.</p>
<p>I hope that isn't too much. Those are the things I noticed from my quick glance.</p></pre>aywot: <pre><p>Not too much at all. I appreciate it. I was under the impression that each project should be a repository but now realize this was a bit silly. As a stupid little example it could very well sit in a subdir. I will fix that as well!</p></pre>redditbanditking: <pre><p>Go can use <a href="https://golang.org/pkg/testing/#hdr-Examples" rel="nofollow">Example</a> tag you can add to your tests (also you are missing tests). Godoc will create a special collapsible note like <a href="https://godoc.org/github.com/go-redis/redis#ex-package--CustomCommand" rel="nofollow">this</a>.</p></pre>Killing_Spark: <pre><p>Some thoughts here:
What youve got there isnt really a queue. A queue would release the items after they are finished (yours would grow indefinitly).</p>
<p>I would do it like this: have two seperate slices, one for waiting workers and one for running workers. If a worker gets scheduled put it into the running slice. After the worker is finished remove it entirly. </p>
<p>Another thing would be, that your queue is doing something called busy-waiting (ok not really because of the sleep but still) you could try to use channels there, to be notified, when a worker is finished or a new worker has been added to the queue. </p>
<p>One last thing: why do you pass the workerinfo to the Start() method? Maybe im missing something here but i'd think thats not good (malicious workers could change the values and break your queue) </p>
<p>Overall a good project though, i hope you have some fun learning :) </p></pre>aywot: <pre><p>Yes I was going to add the code to remove items from the queue in the morning :) This is afterall very incomplete. I was just hoping to get any advice if im doing bad coding practices so I don't go too far with bad habits. But no one is yelling at me so far!</p>
<p>I am very glad you bring up me passing *WorkerInfo to the Start() method (a pointer in fact). This was my solution to how to notify the job queue when the task has completed, since that method can now access and modify it's parent WorkerInfo.Finished bool to True. Then when the queue checks up on everything, it can clean the Finished jobs from queue!</p>
<p>I felt pretty clever figuring this part out but Im guessing this is where you're going to tell me I should use channels to do the back-notify? :)</p>
<p>Thank you for your time, I appreciate this</p></pre>Killing_Spark: <pre><p>Concurrency is a pretty complex topic so good job for finding a working solution on your own. </p>
<p>Having the user do this is a bad pratice. Generally you want your library to be used by only usind the defined API. The user shouldnt see the inner implementation of your library. i would just start the method StartNextJob as a new goroutine (instead of the worker.Start). That way you can call a method RemoveWorker() after the worker.Start has finished. Im at my phone right now so giving good examples are hard, i can get back to you if i this explanation wasnt understandable</p>
<p>Other than that i see no mayor problems with your code though :) </p>
<p>Edit: just thought Maybe i should explain why its seen as bad pratice, heard that would be constructiv :D</p></pre>aywot: <pre><p>This is the stuff I was hoping I'd get from some bored programmers! Perfect, thank you! I completely understand your reasoning here and it makes sense. I am not quite seeing what starting StartNextJob as a goroutine would solve in terms of the back-notify without exposing inner implementation. But that could be because its almost 2am. I will sleep on this and take a fresh look in the morning</p></pre>Killing_Spark: <pre><p>Here is an example:
<a href="https://play.golang.org/p/KASHv7_r1Z" rel="nofollow">https://play.golang.org/p/KASHv7_r1Z</a></p>
<p>The point is, the Worker shouldn't even be aware, that he is being run by a queue. The only thing he should be concerned with, is executing his "Start()" method. By making the method of the queue a goroutine rather then the workers method, the Queue has the control over the execution-flow and can react properly, without concerning the worker with these details.</p>
<p>Edit: This example is a bit ugly but I think it demonstrates the mechanism good enough :) </p></pre>aywot: <pre><p>So I actually did not look at your example first before trying to reason with your advice myself. Like I said I was super tired when I responded last but I woke up the next morning and just had a total "duh" moment. Im embarassed I didn't think to do it this way first. No funky call backs-stuff required, just pass the method as a go function and do my clean up in there after it .Work()... duhhh! Already changed this in the code. Now im going to work on using channels to communicate progress and such. I freaking LOVE golang. This is the most fun Ive had since i was 9 and just found Visual Basic, haha. Feel like a kid again</p></pre>Killing_Spark: <pre><p>Glad i was if any help. Keep that spirit that you have, i love it :D</p></pre>snippet2: <pre><p><a href="https://nathanleclaire.com/blog/2014/02/21/how-to-wait-for-all-goroutines-to-finish-executing-before-continuing-part-two-fixing-my-ooops/" rel="nofollow">https://nathanleclaire.com/blog/2014/02/21/how-to-wait-for-all-goroutines-to-finish-executing-before-continuing-part-two-fixing-my-ooops/</a></p></pre>adamcolton: <pre><p>Try using the <a href="https://github.com/golang/lint" rel="nofollow">golint</a> tool. It will both help with the code as well as find the parts of the code that should be documented. You should follow the godoc standard for documenting your code. The best part of doing so (imo) is that <a href="https://godoc.org/github.com/thegtproject/jobqueue" rel="nofollow">your project</a> will be documented on godoc.org.</p>
<p>I would also add some tests. I'll add that I really like the <a href="https://github.com/stretchr/testify/tree/master/assert" rel="nofollow">assert</a> package to help with testing, but that's a matter of personal taste.</p>
<p>If the package is intended as a library, you shouldn't print to stdOut. I would log the messages that you're printing, but that's only one option.</p></pre>aywot: <pre><p>Absolutely, all these stdout prints are just for testing as I learn to program with Go :) Slowly but surely I plan to do all you have mentioned</p></pre>jerf: <pre><p>You may find it educational to ponder the difference between your queue and a channel carrying commands/jobs, perhaps with a buffer. I'm not saying there aren't <em>any</em> differences, but you may still find it educational to ponder as there are probably <em>fewer</em> differences than you might initially realize.</p>
<p>I think <a href="/r/golang" rel="nofollow">/r/golang</a> gets a job queue at least once every few weeks, but speaking for myself I've never used an internal-to-Go queue. Channels always seem to do the job for me in my real code.</p>
<p>(I say "internal-to-Go" because there are abundant reasons to use <em>external</em> queue systems with your Go program.)</p></pre>aywot: <pre><p>This is exactly the kind of comment I was hoping for posting this. I find this the fastest way to learn when others can steer me into the right answers.</p>
<p>So let me preface this with- i've been learning go for about 2 weeks now. So im still taking everything in and am not very knowledgeable on channels yet.</p>
<p>What I think I know... Channels are simply a guaranteed space. I think the word is atomic. Meaning that if a process wants to read or write to it, it will be guaranteed that the operation completes without something else changing it at the same time. Knowing that, I'd say that safety is exactly why channels would be useful in my queue here cause there is nothing providing that safety from my rather ugly state variables in the struct. So something can happen when im trying to change the state from one thing to another and cause issues.</p>
<p>That said, i've kind of been intimidated by channels and havnt quite jumped into learning them yet, hence my implementation without :) But I will very probably switch to a channel based system in the near future. Im just using this project to learn go, and im having a blast.</p></pre>jerf: <pre><p>Ah, I see I can elaborate a little then.</p>
<p>The relevant feature about channels here is that any number of goroutines can write to them, and any number can read from them. So it's really easy to create a <code>chan Job</code>, spawn any number of goroutines you want to do the jobs, have them enter an infinite for loop that reads a job and does it, and exit if the channel closes. Then, to push a job on to the list, you have something else send into that channel. Making this "professional-grade" involves figuring out how to deal with a job that errors or panics.</p>
<p>The only performance thing you might want to watch out for is to ensure that running a Job is a significantly larger task than popping one element off of a channel. Popping an element off of a channel is somewhere in the dozens of nanoseconds most of the time, I believe, so that's not a hard bar to leap, but one thing I've seen a few times here on <a href="/r/golang" rel="nofollow">/r/golang</a> is people getting fooled by the way concurrency in Go performs when they try to make their jobs something as simple as adding two integers together, in which case the Go concurrency system looks "slow" because you're paying dozens of nanoseconds to run code whose payload runs in under one nanosecond, resulting in the system imposing a multiple order-of-magnitude slowdown on the overall performance. That's a degenerate case, though, to which the answer is basically "don't do that".</p>
<p>The primary difference between a queue and a channel is that if the channel is full (be it buffered or otherwise), attempting to place a job on to the queue will block until there is a worker available. From the point of view of the thing filling the job queue, this may seem suboptimal, because if it could just stuff an entry on to a queue and keep going, it would be able to do other things, right? Well.... in practice, if you want to do "other things" too you really ought to be doing <em>those</em> things in a goroutine of their own. Meanwhile, the fact that the channel slows down the poster is actually a <em>good</em> thing; it's a form of backpressure, which means you won't fill up a queue with jobs that can't possibly be started yet. Considering the program <em>holistically</em>, that's almost always a good thing.</p>
<p>So, generally speaking, I find a channel to be more useful in every way for a job queue system. There is a place where a more manual queuing system could come in handy, I just haven't hit it yet. </p>
<p>And I'm not trying to be critical either, just to be clear, but give some thoughts on how this all fits together. Best of luck as you continue learning!</p></pre>bru7us: <pre><p>As others have basically sad, you're reaaaallly going to want to get familiar with channels in this kind of solution, they will give you a lot of power and safety.</p>
<p>One major issue I see after a quick glance is that you are using a slice in a concurrent world. You will definitely need to consider a sync.Mutex if you want to continue with the slice... think about the race that happens when you add your deletion code and one job is being deleted from the slice at the same time as another job is being started with a slice index that may or may not have now changed.</p>
<p>Also check out the context package, it's a great way to manage how and when your workers exit (assuming that they are long-living workers), but it will require an initial understanding of channels.
Francesc Campoy has a great YouTube channel called JustForFunc - I highly recommend watching all of the videos as your understanding of the language allows you to. But specifically check out <a href="https://www.youtube.com/watch?v=LSzR0VEraWw" rel="nofollow">Episode 9</a> and <a href="https://www.youtube.com/watch?v=8M90t0KvEDY" rel="nofollow">10</a> for coverage on context.</p></pre>aywot: <pre><p>Will do! Thank you! I did get familiar with channels yesterday and have branched a change to that <a href="https://github.com/thegtproject/jobqueue/tree/channel" rel="nofollow">https://github.com/thegtproject/jobqueue/tree/channel</a> Channels arent as intimidating as i thought they would be. Today I hope to accomplish getting rid of the run loop in favor of using an "event" channel that I will use for blocking and then any go routine can send an event for processing. Much more elegant. so fun!</p></pre>
