New watch api #122

asdine · 2019-05-22T15:00:23Z

This PR refreshes the Watch API according to what has been decided in issue #112

The Watch API is now accessible under the following endpoint:

// URL
POST /rulesets/?watch

// Optional query params
?revision

// Payload
{
  "paths": [
    "pathA",
    "pathB",
   ],
  "revision": 10
}

The body specifies which paths you want to watch.
An empty body means watch everything.

This PR also contains a few improvements in the server package

rogpeppe

This looks really nice in general, thanks. LGTM modulo a bunch of minor comments and suggestions, and one likely bug...

api/etcd/rulesets_test.go

api/etcd/watch.go

rogpeppe · 2019-05-22T15:40:33Z

api/etcd/watch.go

-// Watch the given prefix for anything new.
-func (s *RulesetService) Watch(ctx context.Context, prefix string, revision string) (*api.RulesetEvents, error) {
+// Watch a list of paths for changes and return a list of events. If the list is empty or nil,
+// watch all paths. If the revision is empty, watch from the latest revision.


Suggested change

// watch all paths. If the revision is empty, watch from the latest revision.

// watch all paths. If the revision holds a non-negative decimal number, Watch

// will block until the revision is greater than that number, otherwise

// it will return immediately with the initial events.

?

api/etcd/watch.go

http/server/api.go

rogpeppe · 2019-05-22T17:14:40Z

http/server/api.go

@@ -50,6 +39,15 @@ func (s *rulesetAPI) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		s.get(w, r, path)
 		return
 	case "POST":
+		if _, ok := r.URL.Query()["watch"]; ok {
+			if len(path) != 0 {


I wonder if it might be nicer to have an explicit path to use to watch (e.g. /watch-rulesets), rather than a POST to /rulesets. Not sure.
🌷

Actually, we need a POST anyway because we need to send a body in the request

It was really the re-purposing of the endpoint for both purposes that felt odd to me. I don't mind much - we should really just be using gRPC and then we wouldn't need to care about such trivialities :)

http/server/api.go

http/server/api_test.go

http/server/handler.go

api/etcd/rulesets_test.go

api/etcd/watch.go

Co-Authored-By: Roger Peppe <[email protected]>

asdine · 2019-06-05T10:05:25Z

🆙

yaziine

👍

tealeg

Couple of minor things.

api/etcd/watch.go

Co-Authored-By: Geoffrey J. Teale <[email protected]>

rogpeppe

Looks great in general, with one simplification suggestion, and perhaps it's worth improving that Watch test?

rogpeppe · 2019-06-05T13:19:25Z

api/etcd/watch.go

+
+// shouldIncludeEvent reports whether the given event should be included
+// in the Watch data for the given paths.
+func (s *RulesetService) shouldIncludeEvent(ev *clientv3.Event, paths []string) bool {


This could be a little bit simpler, I think:

if len(paths) == 0 { return true } key := string(ev.Kv.Key) key = key[:strings.Index(key, versionSeparator)] for _, path := range paths { if s.rulesPath(path) == key { return true } } return false

Also, I'm a bit concerned that this code will panic if the key doesn't contain versionSeparator. How sure are we of that?

Put is the only function responsible for writing in that path, I'd say that as long as Put is tested it should be fine, but I'm open to suggestions

rogpeppe · 2019-06-05T13:31:21Z

api/etcd/watch_test.go

+			var events api.RulesetEvents
+			var rev int64
+			var watchCount int
+			for len(events.Events) != len(test.expected) && watchCount < 4 {


ISTM that this won't fail if the watcher produces more events than you want. Perhaps the loop should wait for a while to check that no more arrive?

It can't produce more events because each call to createBoolRuleset in the goroutine above will produce exactly one event

rogpeppe · 2019-06-05T13:33:29Z

api/etcd/watch_test.go

+			for len(events.Events) != len(test.expected) && watchCount < 4 {
+				evs, err := s.Watch(ctx, api.WatchOptions{Paths: test.paths, Revision: rev})
+				if err != nil {
+					if err != nil {


This condition looks redundant to me.

rogpeppe · 2019-06-05T14:10:05Z

api/etcd/watch_test.go


-		r := rule.New(rule.True(), rule.BoolValue(true))
+				time.Sleep(time.Second)


Do you actually need the watch statement to execute before this code? How about adding some rulesets, wait for an event to arrive, then adding some more, so you check both the initial case and the later case? (I'm not entirely sure of the intended semantics when there are rulesets already there though - what's meant to happen?)

rogpeppe · 2019-06-05T14:10:39Z

api/service.go

+	// List of paths to watch for changes.
+	// If the slice is empty, watch all paths.
+	Paths []string
+	// Indicates from which revision start watching.


Suggested change

// Indicates from which revision start watching.

// Indicates from which revision to start watching.

rogpeppe · 2019-06-05T14:12:00Z

http/server/api.go

@@ -50,6 +39,15 @@ func (s *rulesetAPI) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 		s.get(w, r, path)
 		return
 	case "POST":
+		if _, ok := r.URL.Query()["watch"]; ok {
+			if len(path) != 0 {


It was really the re-purposing of the endpoint for both purposes that felt odd to me. I don't mind much - we should really just be using gRPC and then we wouldn't need to care about such trivialities :)

rogpeppe · 2019-06-05T14:14:31Z

http/server/api_test.go

-		{"UnexpectedError", "/rulesets/someprefix?list", http.StatusInternalServerError, new(api.Rulesets), api.ListOptions{}, errors.New("unexpected error")},
-		{"InvalidLimit", "/rulesets/someprefix?list&limit=badlimit", http.StatusBadRequest, nil, api.ListOptions{}, nil},
+		{"InvalidCursor", "/rulesets/?list&cursor=abc123", http.StatusBadRequest, new(api.Rulesets), api.ListOptions{Cursor: "abc123"}, api.ErrInvalidCursor},
+		{"UnexpectedError", "/rulesets/?list", http.StatusInternalServerError, new(api.Rulesets), api.ListOptions{}, errors.New("unexpected error")},


Why is this an internal server error rather than a bad request?

The 500 will not be caused by the URL by the error errors.New("unexpected error") that will be returned by the mock.
Basically, it's to simulate an unexpected error (network, etc.) happening in the store.

asdine added 10 commits May 22, 2019 16:15

Changed Watch signature

c584f46

Change etcd watch implementation

767805a

Return the latest revision when a timeout occurs

8018d3a

Test watch api

feb7276

Fix mock

be77fd4

Simplify server handler

5fd73c6

Fix list tests

8569a17

Decode paths from the request body

9b1adf5

Support empty body

42405cd

Improve docstring

067f696

asdine requested review from tealeg, rogpeppe and yaziine May 22, 2019 15:00

rogpeppe suggested changes May 22, 2019

View reviewed changes

asdine added 5 commits May 23, 2019 15:59

Improve etcd watch docstring

2ef744a

Change revision type from string to int64

acc0cca

Change mock watch signature

64597bd

Return an error when receiving a malformed revision

1f7f01e

Change RulesetEvents revision from string to int64

3705b6b

yaziine reviewed May 23, 2019

View reviewed changes

api/etcd/rulesets_test.go Show resolved Hide resolved

api/etcd/watch.go Outdated Show resolved Hide resolved

api/etcd/watch.go Show resolved Hide resolved

asdine and others added 10 commits May 23, 2019 16:12

Support revision as int64

41d1abd

Fix etcd watch tests

1607250

Fix http watch tests

5180456

Simplify etcd watch key selection

8f2b579

Fix slice out of range bug

7b1bc5d

Ignoring malformed payload

f717e73

Mark test helpers as test helpers

2acc01d

Explain the time.Sleep in the watch test

952546b

Apply suggestions from code review

8633e8c

Co-Authored-By: Roger Peppe <[email protected]>

Fix regression

db1cb26

asdine added 8 commits May 23, 2019 17:00

Change Rulesets revision type from string to int64

07d9e3d

Use r.Form instead of r.URL.Query

04f26fc

Fix missing ParseForm in ServeHTTP

8b0e35c

Add WatchOptions and changed Watch signature

83a4367

Implement WatchOptions support for etcd

d0c1b57

Use WatchOptions for revision and paths in HTTP api

e271428

Improve happy path

38b88a1

Simplify timeout handling

f0e5eb9

asdine requested a review from rogpeppe June 5, 2019 10:09

yaziine approved these changes Jun 5, 2019

View reviewed changes

tealeg approved these changes Jun 5, 2019

View reviewed changes

api/etcd/watch.go Outdated Show resolved Hide resolved

api/etcd/watch.go Outdated Show resolved Hide resolved

Apply suggestions from code review

0f2381f

Co-Authored-By: Geoffrey J. Teale <[email protected]>

rogpeppe approved these changes Jun 5, 2019

View reviewed changes

asdine closed this Nov 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New watch api #122

New watch api #122

asdine commented May 22, 2019 •

edited

Loading

rogpeppe left a comment

rogpeppe May 22, 2019

rogpeppe May 22, 2019

asdine May 23, 2019

rogpeppe Jun 5, 2019

asdine commented Jun 5, 2019

yaziine left a comment

tealeg left a comment

rogpeppe left a comment

rogpeppe Jun 5, 2019

asdine Jun 5, 2019

rogpeppe Jun 5, 2019

asdine Jun 5, 2019

rogpeppe Jun 5, 2019

rogpeppe Jun 5, 2019

rogpeppe Jun 5, 2019

rogpeppe Jun 5, 2019

rogpeppe Jun 5, 2019

asdine Jun 5, 2019

-// watch all paths. If the revision is empty, watch from the latest revision.
+// watch all paths. If the revision holds a non-negative decimal number, Watch
+// will block until the revision is greater than that number, otherwise
+// it will return immediately with the initial events.


		r := rule.New(rule.True(), rule.BoolValue(true))
		time.Sleep(time.Second)

	// Indicates from which revision start watching.
	// Indicates from which revision to start watching.

New watch api #122

New watch api #122

Conversation

asdine commented May 22, 2019 • edited Loading

rogpeppe 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

asdine commented Jun 5, 2019

yaziine left a comment

Choose a reason for hiding this comment

tealeg left a comment

Choose a reason for hiding this comment

rogpeppe 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

Choose a reason for hiding this comment

asdine commented May 22, 2019 •

edited

Loading