Pull groups configuration from content-service #26

joselsegura · 2020-06-05T10:44:08Z

Description

This PR includes pulling the groups configuration from the content service and serving that configuration as an smart proxy endpoint.

It also includes a mechanism to periodically update the group configuration.

Fixes #18

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

Testing steps

Regular CI. Increased UT threshold from 0 to 23

quarckster · 2020-06-05T10:46:49Z

config.toml

@@ -11,3 +11,4 @@ enable_cors = false
 [services]
 aggregator = "http://localhost:8080/api/v1/"
 content = "http://localhost:8082/api/v1/"
+groups_poll_time = 60


Why do we need to poll?

We want to update the group configuration periodically to cache it. If we don't poll, we can miss groups configuration changes on the content service

tisnik

LGTM in overall but - don't we need mutex to sync group writes?

joselsegura · 2020-06-05T11:14:34Z

@tisnik good point, I didn't think about it. Do you mean to avoid collisions between reading the configuration and updating it from the content service?

Serhii1011010 · 2020-06-05T11:24:45Z

@tisnik but we have only 1 writing goroutine, don't we?

codecov-commenter · 2020-06-05T15:18:28Z

Codecov Report

Merging #26 into master will increase coverage by 0.75%.
The diff coverage is 45.61%.

@@            Coverage Diff             @@
##           master      #26      +/-   ##
==========================================
+ Coverage   20.31%   21.07%   +0.75%     
==========================================
  Files           9        9              
  Lines         507      541      +34     
==========================================
+ Hits          103      114      +11     
- Misses        391      413      +22     
- Partials       13       14       +1

Impacted Files	Coverage Δ
server/server.go	`18.51% <12.50%> (-1.49%)`	⬇️
server/endpoints.go	`58.82% <52.94%> (-1.18%)`	⬇️
smart_proxy.go	`36.14% <57.14%> (+5.98%)`	⬆️
conf/configuration.go	`72.72% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 455a646...89802f7. Read the comment docs.

tisnik

LGTM - just the clarification might be needed please

smart_proxy.go

Serhii1011010 · 2020-06-08T08:39:40Z

I don't really understand why we need to put the same data to the channel in a loop. How is it better than just using a pointer?

Serhii1011010 · 2020-06-08T08:45:22Z

server/server.go

@@ -241,6 +245,28 @@ func (server HTTPServer) proxyTo(baseURL string) func(http.ResponseWriter, *http
 	}
 }

+// getGroups retrives the groups configuration from a channel to get the latest valid one and send the response back to the client
+func (server *HTTPServer) getGroups(writer http.ResponseWriter, request *http.Request) {
+	select {


What's the purpose of this select? It's reading only from one channel, isn't it?

You are right, I don't remember why I put it, but it is not needed at all. TYVM!

Fixed in the last commit

joselsegura · 2020-06-08T08:53:43Z

@Sergey1011010 the channel is used to pass the groups configuration from the "writer" goroutine (updateGroupInfo) to the "reader" one (getGroup HTTP request handler). Every time the reader needs a value, it can receive it from the channel, and using the channel makes the writer unable to write a new configuration (due a "tick") and send the configuration to the channel, so it acts as a synchronization mechanism

Serhii1011010 · 2020-06-08T09:03:10Z

@joselsegura, yes, but why do you need such a complicated mechanism when you can just replace an array at once in writing goroutine and you won't have any race conditions.

joselsegura · 2020-06-08T10:04:54Z

After talking with @Sergey1011010, he convinced me that there is no possibility of having problems using a simple array mechanism, due to there will only be a goroutine writing the array, and given that the slice update operation is atomic, it cannot be possible to have a "readers/writer" problem here.

So, what do you think to returning back to the first proposal instead of using a channel?

tisnik · 2020-06-08T10:45:35Z

After talking with @Sergey1011010, he convinced me that there is no possibility of having problems using a simple array mechanism, due to there will only be a goroutine writing the array, and given that the slice update operation is atomic, it cannot be possible to have a "readers/writer" problem here.

So, what do you think to returning back to the first proposal instead of using a channel?

it's up to you (don't know about the discussion), but the official rule is simple:

if multiple goroutines access a variable concurrently, and at least one of the accesses is a write, then synchronization is required.

And also according to spec:

structured variables of array, slice, and struct types have elements and fields that may be addressed individually. Each such element acts like a variable.

Re atomicity: it's just slightly related to concurrent R/W operations.

joselsegura · 2020-06-08T11:05:18Z

it's up to you (don't know about the discussion), but the official rule is simple:
* if multiple goroutines access a variable concurrently, and at least one of the accesses is a write, then synchronization is required.
And also according to spec:
* structured variables of array, slice, and struct types have elements and fields that may be addressed individually. Each such element acts like a variable.
Re atomicity: it's just slightly related to concurrent R/W operations.

Apparently we are exactly matching the rule statement: we have several goroutines and one (and only one of them) is writing the values.

So, I will keep the current version until you find anything more to comment.

P.S. Plase, resolve the requests for change if you are ok with the updates

Serhii1011010 · 2020-06-08T11:29:13Z

@tisnik but slice stores a pointer to an array and updating this pointer(setting new array to a slice) is atomic, isn't it?

tisnik · 2020-06-08T11:34:50Z

@tisnik but slice stores a pointer to an array and updating this pointer(setting new array to a slice) is atomic, isn't it?

Indeed it might be on most architectures as max.capacity is equal to max int and int is "naturally" selected (32bit or 64bit). And slice stores also length, and capacity. We might assume it might be changed in atomic operation as well. But pointer+length+capacity (which is usually three words) are much more tricky.

joselsegura · 2020-06-08T11:41:46Z

Wow, this scaled quickly :-D Golang internals! Poor Python guy

tisnik

Thanks a lot @joselsegura, LGTM

joselsegura added 2 commits June 5, 2020 11:52

Polling groups from content-service

93fe962

Honor polling time configuration

99b61dd

joselsegura requested review from Bee-lee, bond95, natiiix, quarckster, Serhii1011010 and tisnik as code owners June 5, 2020 10:44

quarckster reviewed Jun 5, 2020

View reviewed changes

tisnik reviewed Jun 5, 2020

View reviewed changes

Fixing unit test problems

97f861c

joselsegura added 2 commits June 5, 2020 17:38

Using a channel for getting groups config from endpoint handler

2ba9ba1

Updated documentation

939ceed

tisnik requested changes Jun 8, 2020

View reviewed changes

smart_proxy.go Outdated Show resolved Hide resolved

Adding a docstring to the getGroups and updateGroupInfo functions

2f6446a

Serhii1011010 reviewed Jun 8, 2020

View reviewed changes

Removing unneeded select

89802f7

tisnik approved these changes Jun 8, 2020

View reviewed changes

quarckster approved these changes Jun 9, 2020

View reviewed changes

joselsegura merged commit a863a05 into RedHatInsights:master Jun 9, 2020

joselsegura deleted the pull_groups branch June 9, 2020 10:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull groups configuration from content-service #26

Pull groups configuration from content-service #26

joselsegura commented Jun 5, 2020

quarckster Jun 5, 2020

joselsegura Jun 5, 2020

tisnik left a comment

joselsegura commented Jun 5, 2020

Serhii1011010 commented Jun 5, 2020

codecov-commenter commented Jun 5, 2020 •

edited

Loading

tisnik left a comment

Serhii1011010 commented Jun 8, 2020

Serhii1011010 Jun 8, 2020

joselsegura Jun 8, 2020

joselsegura Jun 8, 2020

joselsegura commented Jun 8, 2020

Serhii1011010 commented Jun 8, 2020 •

edited

Loading

joselsegura commented Jun 8, 2020

tisnik commented Jun 8, 2020

joselsegura commented Jun 8, 2020

Serhii1011010 commented Jun 8, 2020

tisnik commented Jun 8, 2020

joselsegura commented Jun 8, 2020

tisnik left a comment

Pull groups configuration from content-service #26

Pull groups configuration from content-service #26

Conversation

joselsegura commented Jun 5, 2020

Description

Type of change

Testing steps

quarckster Jun 5, 2020

Choose a reason for hiding this comment

joselsegura Jun 5, 2020

Choose a reason for hiding this comment

tisnik left a comment

Choose a reason for hiding this comment

joselsegura commented Jun 5, 2020

Serhii1011010 commented Jun 5, 2020

codecov-commenter commented Jun 5, 2020 • edited Loading

Codecov Report

tisnik left a comment

Choose a reason for hiding this comment

Serhii1011010 commented Jun 8, 2020

Serhii1011010 Jun 8, 2020

Choose a reason for hiding this comment

joselsegura Jun 8, 2020

Choose a reason for hiding this comment

joselsegura Jun 8, 2020

Choose a reason for hiding this comment

joselsegura commented Jun 8, 2020

Serhii1011010 commented Jun 8, 2020 • edited Loading

joselsegura commented Jun 8, 2020

tisnik commented Jun 8, 2020

joselsegura commented Jun 8, 2020

Serhii1011010 commented Jun 8, 2020

tisnik commented Jun 8, 2020

joselsegura commented Jun 8, 2020

tisnik left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 5, 2020 •

edited

Loading

Serhii1011010 commented Jun 8, 2020 •

edited

Loading