By embedding I meant this:
type ServiceQProperties struct {
...
sync.Mutex
}
which allows you to call p.Lock() instead of p.REMutex.Lock(). It’s just a
personal preference that I was happy about when I learned about it.
One thought here is you could make it a *sync.Mutex and not use a pointer
in these function arguments. Maps vars are pointers so you are doing a
double dereference when accessing RequestErrorLog. This avoids (*sqp) but
there may be extra memory used and work done to copy the entire struct var
as an argument. If you get into performance details I would look at pprof
or package testing benchmarking to decide if it matters, but I try to
choose cleanest code first.
Thanks for sharing your verification approach.
I haven’t gotten far enough into network services to have much to say about
the features although maybe I will in the future. I was curious about this:
The buffered requests are forwarded in FIFO order when the service is
> available next.
wouldn’t you want to do these concurrently if possible?
Matt
On Friday, May 18, 2018 at 11:50:56 AM UTC-5, Ankit Gupta wrote:
>
> Hi Matt,
>
> First of all, thanks so much for the review.
>
> I will revisit the package structure and take care of err handling in a
> more idiomatic way. Few points -
>
> - The mutex is embedded in model.ServiceQProperties. Here is the
> definition in model.balancer_properties.go -
>
> type ServiceQProperties struct {
> ...
> REMutex sync.Mutex
> }
>
> This is used in two places - error logging to service map in
> service_error.go and reading in select_service.go.
>
> - I validated 3 things (all tests were done on a cluster of aws t2 medium
> linux machines) -
>
> a) Program should reduce probability of selection of errored nodes - this
> was done by continously bringing 1 to n-1 services down/up and noting the
> trend of requests sent to each node after every state change. As an
> example, for a 3 node cluster with 1 node down for 2 mins, the probability
> of selection of that node reduced (from 33%) by around 1-2% on each
> subsequent hit.
>
> b) For performance testing, I used apache bench for issuing large number
> of concurrent requests/issuing requests within a time frame and
> benchmarking against nginx. There was a 10% difference on average that is
> attributed to the fact that I preprocess (parse/store) all requests before
> forwarding and that the code is not the most optimzed version right now.
>
> c) For deferred queue functionality, I specifically wanted to test
> scenarios where both active and deferred requests are to be sent out
> simultaneously and if user defined concurrency limits are still valid.
>
> Let me know if I should provide more details on a specific feature.
>
> Thanks,
> Ankit
>
> On Friday, May 18, 2018 at 8:21:12 PM UTC+5:30, matthe...@gmail.com wrote:
>>
>> Hi Ankit, thanks for the Apache license and for sharing here. Here’s a
>> code review.
>>
>> These are my unfiltered opinions and I hope that they are useful.
>>
>> Without looking at any code yet, the packages might not be idiomatic.
>> Packages should contain specific behavior that can be decoupled from the
>> application. These concepts could be represented as files and symbols in
>> package main.
>>
>> SQP_K_LISTENER_PORT might be more regularly represented as
>> SQPKListenerPort.
>>
>> In config_manager_test.go you could embed model.ServiceQProperties and
>> error in type Properties for better readability.
>>
>> The newlines in the functions don’t help readability.
>>
>> Instead of this:
>>
>> if sqp, err := getProperties(getPropertyFilePath()); err == nil {
>> …
>> } else {
>> fmt.Fprintf(os.Stderr…
>>
>> this may be more readable:
>>
>> // or if …; err != nil {
>> sqp, err := getProperties(getPropertyFilePath())
>> if err != nil {
>> fmt.Fprintf(os.Stderr…
>> return
>> }
>> // regular functionality
>>
>> Generally “err == nil” shouldn’t be in the code.
>>
>> In getListener a similar improvement could be made to avoid the
>> unnecessary indentation:
>>
>> if sqp.SSLEnabled == false {
>> return …
>> }
>> // longer behavior
>>
>> In TestWorkAssignment the sqp could be simpler:
>>
>> sqp := model.ServiceQProperty{
>> ListenerPort: “5252”,
>> Proto: “http”,
>> …
>>
>> More if improvement in algorithm.ChooseServiceIndex:
>>
>> if retry != 0 {
>> return …
>> }
>> // longer behavior
>>
>> The else after “if sumErr == 0” is unnecessary.
>>
>> The mutex could be embedded in model.ServiceQProperties.
>>
>> How did you validate this program?
>>
>> Thanks,
>> Matt
>>
>> On Friday, May 18, 2018 at 2:34:20 AM UTC-5, Ankit Gupta wrote:
>>>
>>> Hello gophers,
>>>
>>> I recently built a small HTTP load balancer with capabilities built
>>> around error feedback - https://github.com/gptankit/serviceq
>>> Provides two primary functionalities - deferred request queue and
>>> probabilitically reducing errored nodes selection.
>>>
>>> I am using channel as a in-memory data structure for storing deferred
>>> requests. Would love some feedback on this approach and on the project in
>>> general.
>>>
>>>
>>>
--
You received this message because you are subscribed to the Google Groups
"golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to golang-nuts+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
