Skip to content

fix: ignore update messages from itself#23

Merged
hsluoyz merged 2 commits intoapache:masterfrom
sasakiyori:ignore_self_update
Dec 1, 2022
Merged

fix: ignore update messages from itself#23
hsluoyz merged 2 commits intoapache:masterfrom
sasakiyori:ignore_self_update

Conversation

@sasakiyori
Copy link
Contributor

@sasakiyori sasakiyori commented Oct 11, 2022

Fix: #22

When policy changes, etcd-watcher will Put a message into etcd, then the revision of key will be updated.

resp, err := w.client.Put(context.TODO(), w.keyName, "")

At the same time, etcd-watcher will get this new revision because it is watching this key.

func (w *Watcher) startWatch() error {
	watcher := w.client.Watch(context.Background(), w.keyName)

So in last pr #22 , to prevent consuming the messages put by self, every time etcd-watcher put a message, it will record the revision from the Put response. And in the goroutine for Watch, it will verify the revision of received message is bigger than recorded one or not.

for res := range watcher {
    t := res.Events[0]
    if t.IsCreate() || t.IsModify() {
	  w.lock.RLock()
	  //ignore self update
	  if rev := t.Kv.ModRevision; rev > w.lastSentRev && w.callback != nil {
		  w.callback(strconv.FormatInt(rev, 10))
	  }
	  w.lock.RUnlock()
    }
}

But in the Update function, it only update local recorded revision when Put fails, it means this feature will be never triggered.

func (w *Watcher) Update() error {
	w.lock.Lock()
	defer w.lock.Unlock()
	resp, err := w.client.Put(context.TODO(), w.keyName, "")
        // We should update lastSentRev when put operation succeeds, but not when it fails.
        // if err != nil {
	if err == nil {
		w.lastSentRev = resp.Header.GetRevision()
	}
	return err
}

@coveralls
Copy link

coveralls commented Oct 11, 2022

Coverage Status

Coverage increased (+3.03%) to 80.303% when pulling 9fd1f9e on sasakiyori:ignore_self_update into bb800b6 on casbin:master.

@casbin-bot
Copy link

@tangyang9464 @JalinWang @imp2002 please review

@sasakiyori
Copy link
Contributor Author

oh why the coverage is decreased to 0.0%? I saw the coverage result from CI (Go/test (1.16)) is 85.4%

@hsluoyz
Copy link
Member

hsluoyz commented Oct 11, 2022

@imp2002 plz review

@sasakiyori
Copy link
Contributor Author

Can someone review this PR? @hsluoyz @JalinWang @imp2002

@sasakiyori
Copy link
Contributor Author

oh why the coverage is decreased to 0.0%? I saw the coverage result from CI (Go/test (1.16)) is 85.4%

I don't know why the coverage from coveralls is different from CI. And I think using time.Sleep in the unit test is not graceful. Thus I revert the changes of unit tests.

@imp2002
Copy link

imp2002 commented Dec 1, 2022

LGTM

@hsluoyz hsluoyz merged commit ae49efe into apache:master Dec 1, 2022
@github-actions
Copy link

github-actions bot commented Dec 1, 2022

🎉 This PR is included in version 2.1.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants