watcher keep alive even get 304 not modified #36

kkdeok · 2022-08-03T03:11:56Z

Hi all! I'm really happy to see this project. I'm working on some python project and it use central dogma so this is what I really want! I've test it in my project and I've found the repository watcher is dead after 304 response.

I think it because here. new_latest revision is None when 304 returned so schedule_watch is not triggered. So I moved schedule_watch out from if statement. Could you please check if it makes sense to you? I worried about stackeoverflow but, focused on keeping alive watcher for now.

I look forward to your reply. Thanks!

p.s 저는 한국 개발자입니다. 컨텍스트 공유를 위해 영어로 작성해봤어요. 수고에 진심으로 감사드립니다 :)

centraldogma/repository_watcher.py

ikhoon · 2022-08-05T10:15:21Z

I worried about stackeoverflow but, focused on keeping alive watcher for now.

Good point. It is possible. We can use a loop instead of recursion to avoid stack overflow.
For example:

def _schedule_watch(self, num_attempts_so_far: int) -> None:

    while num_attempts_so_far >= 0:
        if num_attempts_so_far == 0:
            delay = _DELAY_ON_SUCCESS_MILLIS if self._latest is None else 0
        else:
            delay = self._next_delay_millis(num_attempts_so_far)

        time.sleep(delay / 1000)
        num_attempts_so_far = self._watch(num_attempts_so_far)

def _watch(self, num_attempts_so_far: int) -> int:
    if self._is_stopped():
       return -1

    ...
    // return 0 to watch again
    // return num_attempts_so_far + 1 in case a error occur

Are you interested in fixing it as well?

kkdeok · 2022-08-07T13:41:57Z

I worried about stackeoverflow but, focused on keeping alive watcher for now.

Good point. It is possible. We can use a loop instead of recursion to avoid stack overflow. For example:

def _schedule_watch(self, num_attempts_so_far: int) -> None:

    while num_attempts_so_far >= 0:
        if num_attempts_so_far == 0:
            delay = _DELAY_ON_SUCCESS_MILLIS if self._latest is None else 0
        else:
            delay = self._next_delay_millis(num_attempts_so_far)

        time.sleep(delay / 1000)
        num_attempts_so_far = self._watch(num_attempts_so_far)

def _watch(self, num_attempts_so_far: int) -> int:
    if self._is_stopped():
       return -1

    ...
    // return 0 to watch again
    // return num_attempts_so_far + 1 in case a error occur

Are you interested in fixing it as well?

Sure! Thank you for your suggestion. I just updated the code based on your suggestion with some test code. Please take a look the commit

ikhoon

Thanks, @doubleknd26!
I'm looking forward to more feedback on centraldogma-python 😉

codecov-commenter · 2022-08-08T05:31:56Z

Codecov Report

Merging #36 (05b645f) into main (1499b64) will increase coverage by 0.29%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
+ Coverage   93.44%   93.74%   +0.29%     
==========================================
  Files          23       23              
  Lines         702      703       +1     
==========================================
+ Hits          656      659       +3     
+ Misses         46       44       -2

Impacted Files	Coverage Δ
centraldogma/dogma.py	`100.00% <100.00%> (ø)`
centraldogma/repository_watcher.py	`94.11% <100.00%> (+0.78%)`	⬆️
centraldogma/watcher.py	`75.00% <0.00%> (ø)`
centraldogma/data/entry.py	`71.42% <0.00%> (ø)`
centraldogma/exceptions.py	`100.00% <0.00%> (ø)`
centraldogma/data/revision.py	`92.30% <0.00%> (ø)`
centraldogma/query.py	`96.29% <0.00%> (+3.70%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

kkdeok · 2022-08-09T15:32:20Z

@hexoul @minwoox
I still cannot merge it and seems I need to get another approval. Anyone can help me to merge it? If you have any coments, please feel free to add :)

ikhoon · 2022-08-10T02:28:29Z

I still cannot merge

The permission to merge is granted to the maintainers of this project. It is normal that you can't merge this PR.
I'd like to wait for a review from our core maintainer @hexoul who may merge this PR after reviewing.

tests/test_repository_watcher.py

minwoox · 2022-08-10T02:35:17Z

@syleeeee Do you mind adding CLA assistant bot to this repository? 😄

kkdeok · 2022-08-10T04:31:51Z

I still cannot merge

The permission to merge is granted to the maintainers of this project. It is normal that you can't merge this PR. I'd like to wait for a review from our core maintainer @hexoul who may merge this PR after reviewing.

Oh I see. Thanks for the detail :)

hexoul

Thanks for your contribution. Please check comments.

tests/test_repository_watcher.py

hexoul · 2022-08-10T09:44:21Z

tests/test_repository_watcher.py

+
+
+def create_watcher():
+    return RepositoryWatcher(


Changing AbstractWatcher._schedule_watch() affects not only RepositoryWatcher but also FileWatcher. It would be better to add tests at the same time. Furthermore, integration test seems proper to replay the case you reported.

Added tests for FileWatcher.

I tried implements integration test for my case, but I faced that the dogma client returns either timeout exception or response 304 NOT_MODIFIED. Because the timeout exception is not related to the case I reported, I'm not sure the integration test can fully cover it. Nevertheless, I have thought about the integration test code like below. WDYT?

def test_not_modified_repository_watcher(self, run_around_test): timeout_millis = 1000 # we need to pass timeout_millis as a parameter. watcher: Watcher[Revision] = dogma.repository_watcher( project_name, repo_name, "/**", timeout_millis ) # wait until watcher get NOT_MODIFIED at least one. time.sleep(5 * timeout_millis / 1000) commit = Commit("Upsert modify.txt") upsert_text = Change("/path/modify.txt", ChangeType.UPSERT_TEXT, "modified") result = dogma.push(project_name, repo_name, commit, [upsert_text]) assert result.revision == watcher.latest.revision.major

Added tests for FileWatcher.

Looks good! Thanks for kind consideration. 🙏

I have thought about the integration test code like below. WDYT?

Makes sense for me. It would be better to check how many the number of calls of _watch() there is by scheduler, but okay. How about @ikhoon @minwoox ?

Gentle ping @ikhoon @minwoox

@doubleknd26 By the way, the integration test you proposed is not included in changes. Could you add that?

@hexoul Sure! I added it.

The integration test code sounds good to me.
By the way, the code suggestion left above does not seem to be correctly applied.

// return 0 to watch again // return num_attempts_so_far + 1 in case a error occur

centraldogma/repository_watcher.py

minwoox · 2022-08-17T06:07:16Z

@doubleknd26 Could you sign the CLA, please?
https://cla-assistant.io/line/centraldogma-python?pullRequest=36

CLAassistant · 2022-08-20T12:32:28Z

All committers have signed the CLA.

kkdeok · 2022-08-20T13:32:36Z

@doubleknd26 Could you sign the CLA, please? https://cla-assistant.io/line/centraldogma-python?pullRequest=36

Done. Thanks!

hexoul · 2022-08-22T16:39:27Z

Mostly LGTM 🎉 Please check broken tests.

kkdeok · 2022-08-29T13:44:34Z

Mostly LGTM 🎉 Please check broken tests.

Thanks for your comment! I fixed your point and confirmed it works in my forked repo. I hope it works well in here too.

centraldogma/repository_watcher.py

ikhoon · 2022-08-31T07:19:23Z

tests/integration/test_watcher.py

+        result = dogma.push(project_name, repo_name, commit, [upsert_text])
+
+        # wait until watcher watch latest.
+        time.sleep(4 * timeout_second)


tests/integration/test_watcher.py

centraldogma/repository_watcher.py

Co-authored-by: Seunggon Kim <[email protected]>

ikhoon

Thanks a lot! @doubleknd26 🚀❤️

ikhoon · 2022-09-01T07:59:59Z

Please check the lint failure which might be caused by applying suggestions in the comments.

hexoul

Thanks a lot!

hexoul · 2022-09-01T14:00:51Z

@syleeeee Could you check why CLA is pending?

ikhoon · 2022-09-02T05:23:21Z

Could you check why CLA is pending?

I didn't sign which blocks the CLA. 😱

ikhoon · 2022-09-02T05:28:15Z

@minwoox Would you like to review more?
It is a critical bug to use Watch API. I'll release a new version after merging this PR.

hexoul requested review from ikhoon, hexoul and minwoox August 3, 2022 14:08

ikhoon added the defect label Aug 5, 2022

ikhoon added this to the 0.2.0 milestone Aug 5, 2022

ikhoon reviewed Aug 5, 2022

View reviewed changes

centraldogma/repository_watcher.py Outdated Show resolved Hide resolved

ikhoon approved these changes Aug 8, 2022

View reviewed changes

minwoox reviewed Aug 10, 2022

View reviewed changes

tests/test_repository_watcher.py Outdated Show resolved Hide resolved

hexoul requested changes Aug 10, 2022

View reviewed changes

ikhoon modified the milestones: 0.2.0, 0.3.0 Aug 17, 2022

Kideok added 4 commits August 20, 2022 22:30

watcher keep alive even get 304 not modified

5a459eb

make _watch() use loop instead of recursion to avoid stack overflow

6219239

apply check style for new test file

b7d9c23

Add copyright

b69ff86

kkdeok force-pushed the watcher_keep_alive branch from 8fc46cf to b69ff86 Compare August 20, 2022 13:31

apply comments

9eab2a9

Kideok added 2 commits August 29, 2022 22:21

fix style

d15a3b5

fix integration test failure

91ab6d2

Kideok added 3 commits August 31, 2022 10:35

Add integration test for checking NOT_MODIFIED

1c45ef2

fix style

eaa0ff5

consider the case that repo already have revision

cbbba2f

ikhoon reviewed Aug 31, 2022

View reviewed changes

hexoul reviewed Aug 31, 2022

View reviewed changes

tests/integration/test_watcher.py Outdated Show resolved Hide resolved

tests/integration/test_watcher.py Outdated Show resolved Hide resolved

centraldogma/repository_watcher.py Outdated Show resolved Hide resolved

Kideok and others added 3 commits September 1, 2022 16:34

apply comments

a830dc2

Update tests/integration/test_watcher.py

af461d0

Co-authored-by: Seunggon Kim <[email protected]>

Update tests/integration/test_watcher.py

c5edd4a

Co-authored-by: Seunggon Kim <[email protected]>

ikhoon approved these changes Sep 1, 2022

View reviewed changes

fix style

74919f0

hexoul approved these changes Sep 1, 2022

View reviewed changes

empty commit for retry CLA check

05b645f

ikhoon merged commit b162f88 into line:main Sep 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

watcher keep alive even get 304 not modified #36

watcher keep alive even get 304 not modified #36

kkdeok commented Aug 3, 2022

ikhoon commented Aug 5, 2022 •

edited

Loading

kkdeok commented Aug 7, 2022

ikhoon left a comment

codecov-commenter commented Aug 8, 2022 •

edited

Loading

kkdeok commented Aug 9, 2022 •

edited

Loading

ikhoon commented Aug 10, 2022

minwoox commented Aug 10, 2022

kkdeok commented Aug 10, 2022

hexoul left a comment

hexoul Aug 10, 2022

kkdeok Aug 21, 2022

hexoul Aug 22, 2022

hexoul Aug 30, 2022

kkdeok Aug 31, 2022 •

edited

Loading

ikhoon Aug 31, 2022 •

edited

Loading

minwoox commented Aug 17, 2022

CLAassistant commented Aug 20, 2022 •

edited

Loading

kkdeok commented Aug 20, 2022

hexoul commented Aug 22, 2022 •

edited

Loading

kkdeok commented Aug 29, 2022 •

edited

Loading

ikhoon Aug 31, 2022

ikhoon left a comment

ikhoon commented Sep 1, 2022

hexoul left a comment

hexoul commented Sep 1, 2022

ikhoon commented Sep 2, 2022

ikhoon commented Sep 2, 2022

watcher keep alive even get 304 not modified #36

watcher keep alive even get 304 not modified #36

Conversation

kkdeok commented Aug 3, 2022

ikhoon commented Aug 5, 2022 • edited Loading

kkdeok commented Aug 7, 2022

ikhoon left a comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 8, 2022 • edited Loading

Codecov Report

kkdeok commented Aug 9, 2022 • edited Loading

ikhoon commented Aug 10, 2022

minwoox commented Aug 10, 2022

kkdeok commented Aug 10, 2022

hexoul left a comment

Choose a reason for hiding this comment

hexoul Aug 10, 2022

Choose a reason for hiding this comment

kkdeok Aug 21, 2022

Choose a reason for hiding this comment

hexoul Aug 22, 2022

Choose a reason for hiding this comment

hexoul Aug 30, 2022

Choose a reason for hiding this comment

kkdeok Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

ikhoon Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

minwoox commented Aug 17, 2022

CLAassistant commented Aug 20, 2022 • edited Loading

kkdeok commented Aug 20, 2022

hexoul commented Aug 22, 2022 • edited Loading

kkdeok commented Aug 29, 2022 • edited Loading

ikhoon Aug 31, 2022

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon commented Sep 1, 2022

hexoul left a comment

Choose a reason for hiding this comment

hexoul commented Sep 1, 2022

ikhoon commented Sep 2, 2022

ikhoon commented Sep 2, 2022

ikhoon commented Aug 5, 2022 •

edited

Loading

codecov-commenter commented Aug 8, 2022 •

edited

Loading

kkdeok commented Aug 9, 2022 •

edited

Loading

kkdeok Aug 31, 2022 •

edited

Loading

ikhoon Aug 31, 2022 •

edited

Loading

CLAassistant commented Aug 20, 2022 •

edited

Loading

hexoul commented Aug 22, 2022 •

edited

Loading

kkdeok commented Aug 29, 2022 •

edited

Loading