make HttpArchive retry configurable #628

MarcKe · 2025-03-26T12:15:10Z

No description provided.

codecov · 2025-03-26T12:20:11Z

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.90%. Comparing base (26caae1) to head (2166daa).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
pym/bob/archive.py	94.11%	1 Missing ⚠️
pym/bob/utils.py	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #628      +/-   ##
==========================================
+ Coverage   88.77%   88.90%   +0.12%     
==========================================
  Files          49       49              
  Lines       15864    15867       +3     
==========================================
+ Hits        14083    14106      +23     
+ Misses       1781     1761      -20

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

jkloetzke

Thanks. Just some minor comments...

There seems to be an issue with the test execution on Python 3.12. Maybe one of the new test hangs sporadically?

(BTW, improving on the commit message style would be nice. Just take an inspiration on the existing commits. Usually, the commit message should give a rationale why the change is made. Basically a message to our future selves to understand why things had to be changed. 😉 )

jkloetzke · 2025-03-27T20:05:55Z

pym/bob/archive.py

            if info["path"]:
-                entries.append(info["path"].removeprefix(path + "/"))
+                prefix = path + "/"
+                if sys.version_info >= (3, 9):


Could you move this to the utils module as a function and call that from here? We usually try to hide such version compatibility things there...

jkloetzke · 2025-03-27T20:14:05Z

test/unit/test_archive.py

        spec["download"] = "cp {}/$BOB_REMOTE_ARTIFACT $BOB_LOCAL_ARTIFACT".format(self.repo.name)
        spec["upload"] = "mkdir -p {P}/${{BOB_REMOTE_ARTIFACT%/*}} && cp $BOB_LOCAL_ARTIFACT {P}/$BOB_REMOTE_ARTIFACT".format(P=self.repo.name)

+class TestHttpArchiveRetries(Base, TestCase):


Minor nit: to keep things better reviewable, the tests should be a separate commit. I OK merging it this way but something for keep in mind for the future...

jkloetzke · 2025-03-27T20:17:20Z

test/unit/test_archive.py

-        self.dummyFileName = self.__createArtifact(DOWNLOAD_ARITFACT)
-        self.__createArtifact(WRONG_VERSION_ARTIFACT, "0")
-        self.__createBuildId(DOWNLOAD_ARITFACT)
+        super().setUp()


I guess you can remove the method if you just call the base class method. Same for tearDown below...

There is more code following here. I agree with the tearDown()

jkloetzke · 2025-03-27T20:24:46Z

pym/bob/archive.py

+    def _retry(self, request):
+        (ok, result) = self.__retry(request)
+        if ok:
+            return result
+        else:
+            raise result
+


Why not just modify __retry accordingly? AFAICS, all call sites rely on throwing in case of an error.

Also, please separate the cleanup (getting rid of all the identical if/else constructs) from the actual change. Those are two independent changes.

jkloetzke · 2025-03-27T20:27:23Z

pym/bob/archive.py

-            raise result
+        return self._retry(lambda: self._webdav.upload(path, tmp, overwrite))
+
+    def __listdir(self, path):


Why add a new method? There is only one caller, isn't it. Same for __delete and __stat...

That's a good question. I don't remember my thoughts :>

python 3.8 and older do not have removeprefix() for strings

some backends might be quite unreliable, so a single retry is not sufficient making the retries configurable can mitigate this issue

gets rid of the ugly if else construct all errors are propagated by exceptions anyways

the archive cmd relies on those functions. they can also fail with an unreliable server. applying the retry mechanism here might mitigate those issues

unknown name sounds a lil weird

MarcKe · 2025-03-28T09:30:53Z

Thanks. Just some minor comments...

There seems to be an issue with the test execution on Python 3.12. Maybe one of the new test hangs sporadically?

(BTW, improving on the commit message style would be nice. Just take an inspiration on the existing commits. Usually, the commit message should give a rationale why the change is made. Basically a message to our future selves to understand why things had to be changed. 😉 )

Im confused by python 3.12. The tests that I touched both completed...

MarcKe · 2025-03-28T11:24:01Z

Thanks. Just some minor comments...
There seems to be an issue with the test execution on Python 3.12. Maybe one of the new test hangs sporadically?
(BTW, improving on the commit message style would be nice. Just take an inspiration on the existing commits. Usually, the commit message should give a rationale why the change is made. Basically a message to our future selves to understand why things had to be changed. 😉 )

Im confused by python 3.12. The tests that I touched both completed...

It looks like the blackbox test "setup-scripts" did not even start and the coverage is missing. I do not know why. I tested it with pyenv and 3.12.9. The tests just worked.

jkloetzke · 2025-03-28T17:37:25Z

Strange. Not sure what changed. Seems like I'll have to investigate this. Anyway, that should not prevent us from merging the PR.

Thanks.

MarcKe changed the title ~~Mk archive retry~~ make HttpArchive retry configurable Mar 26, 2025

MarcKe force-pushed the mk_archive_retry branch 3 times, most recently from 36320b7 to 171ce70 Compare March 26, 2025 13:01

MarcKe marked this pull request as ready for review March 26, 2025 13:13

jkloetzke reviewed Mar 27, 2025

View reviewed changes

MarcKe added 6 commits March 28, 2025 09:29

archive: fix prefix removal with python < 3.9

d00b4da

python 3.8 and older do not have removeprefix() for strings

archive: allow configurable retries

8f08321

some backends might be quite unreliable, so a single retry is not sufficient making the retries configurable can mitigate this issue

webdav: fix variable name in PartialDownloader

34f849e

archive: make __retry directly raise on fault

970f4b3

gets rid of the ugly if else construct all errors are propagated by exceptions anyways

archive: add retry to more functions

11ccfd9

the archive cmd relies on those functions. they can also fail with an unreliable server. applying the retry mechanism here might mitigate those issues

archive: "undefined name" if archive name is undefined

2166daa

unknown name sounds a lil weird

MarcKe force-pushed the mk_archive_retry branch from b723c8f to 2166daa Compare March 28, 2025 09:13

jkloetzke merged commit d4b9522 into BobBuildTool:master Mar 28, 2025
10 of 11 checks passed

make HttpArchive retry configurable #628

make HttpArchive retry configurable #628

Uh oh!

Conversation

MarcKe commented Mar 26, 2025

Uh oh!

codecov bot commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jkloetzke left a comment

Choose a reason for hiding this comment

Uh oh!

jkloetzke Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

jkloetzke Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

jkloetzke Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

MarcKe Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

jkloetzke Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

jkloetzke Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

MarcKe Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

MarcKe commented Mar 28, 2025

Uh oh!

MarcKe commented Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkloetzke commented Mar 28, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Mar 26, 2025 •

edited

Loading

MarcKe commented Mar 28, 2025 •

edited

Loading