Patching for Watcher and http client #5

linxGnu · 2018-11-21T05:00:30Z

Fix go routine spawning by cycling call between scheduleWatch and doWatch
Prevent spawning too much go routine to notify listener in case listener fail to catch up. This also prevents notifying >2 revisions at the same time (ambiguous order)
Drain up response (in http client do) when server return malform data and expected response not set
Refactor code

- Fix go routine spawning by cycling call between scheduleWatch and doWatch - Prevent spawning too much go routine to notify listener in case listener fail to catch up. This also prevents notifying >2 revisions at the same time (ambiguous order) - Drain up response (in http client do) when server return malform data and expected response not set - Refactor code

codecov-io · 2018-11-21T05:03:33Z

Codecov Report

Merging #5 into master will increase coverage by 1.07%.
The diff coverage is 70.87%.

@@            Coverage Diff             @@
##           master       #5      +/-   ##
==========================================
+ Coverage   33.24%   34.31%   +1.07%     
==========================================
  Files          20       20              
  Lines        1501     1527      +26     
==========================================
+ Hits          499      524      +25     
- Misses        901      902       +1     
  Partials      101      101

Impacted Files	Coverage Δ
dogma.go	`68.02% <50%> (+4.21%)`	⬆️
watch_service.go	`68.72% <72.22%> (-0.73%)`	⬇️
dogma_util.go	`73.17% <80%> (+4.87%)`	⬆️

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 836d622...55aa63e. Read the comment docs.

consts.go

dogma.go

minwoox

Sorry for taking a long time to review this.
I really appreciate this. :)

minwoox · 2018-11-28T05:03:58Z

dogma.go

@@ -215,29 +215,28 @@ func (c *Client) do(ctx context.Context, req *http.Request, resContent interface

 	res, err := c.client.Do(req)
 	if err != nil {
-		select {
-		case <-ctx.Done():
-			return nil, ctx.Err()


the context's error will be probably more useful?

When context is cancelled or error, res, err would hold the error. So we don't need ctx.Err() here

There's a chance that it gets an error response and the user cancels the ctx before the code reaches here.
Please see https://github.com/google/go-github/blob/master/github/github.go#L484 this.
But it's probably no big deal so we can just return the original err.

Yes, in case user cancel the ctx first, then it's his choice to cancel whole process and stop. I think it's no big deal too.

minwoox · 2018-11-28T05:09:09Z

dogma.go

+		// see also:
+		// - https://github.com/google/go-github/pull/317
+		// - https://forum.golangbridge.org/t/do-i-need-to-read-the-body-before-close-it/5594/4
+		io.CopyN(ioutil.Discard, res.Body, 512)


https://github.com/google/go-github/blob/master/github/github.go#L501
The current version deprecates this feature. Could you recheck it, please?

I did a test on this when building some production and confirm that it still remains.

Could you also change this as well please? :-)
google/go-github#847

you mean add more refence link to comment @minwoox

Ah I mean that we do not have to drain up 512 bytes since it was introduced in 1.7. So we can revert this to as defer res.Body.Close().
Please check out google/go-github#843
and golang/go@18072ad
:-)

@minwoox Yeah, currently we are doing json decoder.

in case server response with additional malform data, then decoder won't help us.

the other case is unknown status which we refuse to consume or processing.
Both of case seem not happened in real production being since server always return well format data. However, because we are doing client, we should just ignore what server response and handle all malform things. WDYT @minwoox

I agree with you on that we have to deal with the case when the response data is malformed.
I have thought about for a while why google/go-github removes the codes introduced. I believe that

the client is only sending the request to the Github which emits the controlled responses by them.

they will get to know easily when the response is modified if they do not have this trick by looking at the new connection count so that they can notice the response is changed and fix it.

they just want to make it simple by removing the code because there's no problem when the condition meets the above situation.

I was just not sure that we still need this trick when I found that the codes are gone.

Perhaps we do not have useful metrics on Central Dogma, we'd better have this trick.

@huydx, @trustin, @hyangtack You guys have any opinion on this?

OK with keeping this as long as it's well commented.

watch_service.go

minwoox · 2018-11-28T05:26:19Z

watch_service.go

@@ -45,9 +43,13 @@ type commitWithEntry struct {

 func (ws *watchService) watchFile(ctx context.Context, projectName, repoName, lastKnownRevision string,
 	query *Query, timeout time.Duration) <-chan *WatchResult {


As we discussed before, a user is expecting more than one watch result when he/she gets a channel as the return value. So how about making this method return just *WatchResult (not the channel) and renaming it to watchFileOneTime.

And remove the FileWatcher and provide watchFile which returns <-chan *WatchResult (inside the method it keeps watching)? What do you think?

As we discussed before, a user is expecting more than one watch result when he/she gets a channel as the return value. So how about making this method return just *WatchResult (not the channel) and renaming it to watchFileOneTime.

Yes it's, and I would make it at next PR. This PR is for FileWatcher only. I think we could keep it and reuse for watchService :)

minwoox · 2018-11-28T05:59:30Z

watch_service.go

@@ -276,56 +316,49 @@ func (ws *watchService) repoWatcher(projectName, repoName, pathPattern string) (

 func (w *Watcher) start() {
 	if atomic.CompareAndSwapInt32(&w.state, initial, started) {
-		w.scheduleWatch(0)
+		go w.scheduleWatch()


Inside of the scheduleWatch() it spawns another Go routine as well. So how about moving the Go routine in watchRequest() out to watchFile and watchRepo so that this watches with only one Go routine?

Since Watcher start only called once. So each watcher has only one daemon to do watch file and notify listeners (which are running on others routines) about changes

Yeah but, watchSchedule() calls doWatch() and doWatch() calls doWatchFunc().
In doWatchFunc(), watchFile is called, and watchRequest() is called which spawns another Go routine.
Am I missing something? :)

Yeah you are right. But it's blocking until watchRequest() done, then no more spawn.
In short, there is only one additional routine, this routine do job and watcher daemon wait for it until it done. So it's fine, no routine leak.

It's the problem of old design, so this patch only focus on Watcher. Client and watchService should be next PR. WDYT @minwoox?

Alright~! Let's fix the problem of the old design in next PR. Thank you~!

Thank you very much for hard review ^_^

Just don't know much about Golang. 😄

trustin

Mostly nits. Nice work :-)

trustin · 2018-11-29T03:39:46Z

consts.go

+)
+
+var (
+	ErrLatestNotSet = fmt.Errorf("latest is not set yet")


(ErrLatestIsNotSet and ErrWatchedIsClosed) or (ErrLatestNotSet and ErrWatcherClosed) for consistency.

consts.go

watch_service.go

Refactor naming and comment

linxGnu · 2018-11-29T05:27:57Z

I have refactor code to address comment from @trustin. PTAL :)
Thank you 🙇

trustin

Thank you!

minwoox · 2018-11-29T08:55:34Z

Thanks a lot. @linxGnu

Fix drainup

afcf9e9

linxGnu changed the title ~~Various patching for watch_service~~ Patching for Watcher and http client Nov 21, 2018

Update watcher delay

e158b02

trustin requested review from minwoox and hyangtack November 22, 2018 06:39

hyangtack reviewed Nov 27, 2018

View reviewed changes

consts.go Outdated Show resolved Hide resolved

dogma.go Outdated Show resolved Hide resolved

Fix typo

ae5ea79

linxGnu requested a review from trustin as a code owner November 27, 2018 08:02

minwoox requested changes Nov 28, 2018

View reviewed changes

Fix typo

62b399b

minwoox reviewed Nov 28, 2018

View reviewed changes

minwoox approved these changes Nov 28, 2018

View reviewed changes

Update consts.go

b3b3bec

trustin requested changes Nov 29, 2018

View reviewed changes

Address comment

55aa63e

Refactor naming and comment

trustin approved these changes Nov 29, 2018

View reviewed changes

hyangtack approved these changes Nov 29, 2018

View reviewed changes

minwoox merged commit 1c98e8b into line:master Nov 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patching for Watcher and http client #5

Patching for Watcher and http client #5

linxGnu commented Nov 21, 2018

codecov-io commented Nov 21, 2018 •

edited

Loading

minwoox left a comment

minwoox Nov 28, 2018

linxGnu Nov 28, 2018

minwoox Nov 28, 2018

linxGnu Nov 28, 2018

minwoox Nov 28, 2018

linxGnu Nov 28, 2018

minwoox Nov 28, 2018

linxGnu Nov 28, 2018

minwoox Nov 28, 2018

linxGnu Nov 28, 2018 •

edited

Loading

minwoox Nov 28, 2018 •

edited

Loading

minwoox Nov 28, 2018

minwoox Nov 28, 2018 •

edited

Loading

trustin Nov 29, 2018

minwoox Nov 28, 2018

minwoox Nov 28, 2018

linxGnu Nov 28, 2018

minwoox Nov 28, 2018 •

edited

Loading

linxGnu Nov 28, 2018

minwoox Nov 28, 2018

linxGnu Nov 28, 2018 •

edited

Loading

minwoox Nov 28, 2018

linxGnu Nov 28, 2018

minwoox Nov 28, 2018

trustin left a comment

trustin Nov 29, 2018

linxGnu commented Nov 29, 2018

trustin left a comment

minwoox commented Nov 29, 2018

		@@ -45,9 +43,13 @@ type commitWithEntry struct {

		func (ws *watchService) watchFile(ctx context.Context, projectName, repoName, lastKnownRevision string,
		query Query, timeout time.Duration) <-chan WatchResult {

Patching for Watcher and http client #5

Patching for Watcher and http client #5

Conversation

linxGnu commented Nov 21, 2018

codecov-io commented Nov 21, 2018 • edited Loading

Codecov Report

minwoox left a 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

linxGnu Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

minwoox Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minwoox Nov 28, 2018 • edited Loading

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

minwoox Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linxGnu Nov 28, 2018 • edited Loading

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

trustin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linxGnu commented Nov 29, 2018

trustin left a comment

Choose a reason for hiding this comment

minwoox commented Nov 29, 2018

codecov-io commented Nov 21, 2018 •

edited

Loading

linxGnu Nov 28, 2018 •

edited

Loading

minwoox Nov 28, 2018 •

edited

Loading

minwoox Nov 28, 2018 •

edited

Loading

minwoox Nov 28, 2018 •

edited

Loading

linxGnu Nov 28, 2018 •

edited

Loading