Load checkpoints in background

[kozlovsky]

This PR fixes #7032

[drew2a]

The string constant "start" used at least twice in the same context, so it would be nice if it will be extracted to a const.

[kozlovsky]

I considered this, but currently, Tribler does not have a tradition of defining constants for task names. Constants are not used in any invocation of register_task / cancel_pending_task. It looks strange to set it just for the "start" task but not for, say, the "download_states_lc" task and then for other tasks, and it is unclear where to stop. It looks like this change is slightly out of the scope of the current PR.

[drew2a]

I agree that the Tribler does not have a tradition of defining constants for task names.

You introduces the new task In this PR, called "start" and for this change (for this task) we can apply best practice and extract the string to a constant.

It looks strange to set it just for the "start" task but not for, say, the "download_states_lc" task and then for other tasks

For me it doesn't look strange.

A refactoring of the whole source base to apply "the new tradition of defining constants for task names" is indeed out of scope of the current PR.

But the actual changes are within the scope of the current PR.

[kozlovsky]

I added a class-level constant for a task name

[drew2a]

🙏

[drew2a]

It would be nice to see all these string constants extracted as a consts to decrease a duplication and decrease a chance of misprinting while editing etc.

[kozlovsky]

I agree that static API is less error-prone, but on the other side, I think it is important to use a consistent approach to all data elements. It is strange to have constants just for some fields.

Like, if we have a dedicated constant for downloads['checkpoints']['total'] key name, why do we not have a similar constant for download["channel_download"] or download["speed_down"]? If we want to change our approach for working with keys of JSON elements, we need to apply it consistently to all keys.

[drew2a]

I agree that static API is less error-prone, but on the other side, I think it is important to use a consistent approach to all data elements. It is strange to have constants just for some fields.

Extracting fields to the const is extremely easy task (thank to PyCharm), and this action noticeably improves the codebase.
I can do it for you if you want :)

[kozlovsky]

If I correctly understand what you want, I'm afraid I have to disagree with this change.

This is how I see what you are suggesting:

• For JSON keys, string constants should be used instead of plain string values in Swagger definitions and the endpoint's code.
• The GUI code should also use the same string constants for JSON keys, or extracting the constants has little sense.
• To be usable on the GUI side, string constants for JSON keys should be moved into a separate module (like part of the common package we had before) and imported both from Core and GUI.
• This new module will contain hundreds of JSON keys from multiple endpoints. The downloads_endpoint alone has about 50 different JSON keys used on different levels of the response structure. In the original JSON structure, the nested keys are located together, and in the new module, they will be mixed with keys for other endpoints in a flat list.
• The resulting code will have an additional level of indirection, which make all JSON-related code harder to read.
• An intermediate state after introducing the partial changes will look even uglier when some JSON keys are listed in a separate thesaurus and other keys are hard coded.

In the end, we will get a half-backed attempt to make JSON code slightly more statically typed with code much harder to read than now and only slightly less error-prone (if at all, as using JSON keys from a vast list of constants listed in a separate module introduces new types of possible coding errors).

If we want to make code better statically typed, we should switch from raw JSON to something like Pydantic and use FastAPI to automatically generate Swagger definitions from Pydantic structures. It is much better than replacing hardcoded keys with constants in all JSON structures.

[drew2a]

This is what I'm asking for:

diff --git a/src/tribler/core/components/libtorrent/restapi/downloads_endpoint.py b/src/tribler/core/components/libtorrent/restapi/downloads_endpoint.py
index c1170cd..acdb705 100644
--- a/src/tribler/core/components/libtorrent/restapi/downloads_endpoint.py
+++ b/src/tribler/core/components/libtorrent/restapi/downloads_endpoint.py
@@ -37,6 +37,12 @@ from tribler.core.utilities.simpledefs import (
 from tribler.core.utilities.unicode import ensure_unicode, hexlify
 from tribler.core.utilities.utilities import froze_it
 
+ALL_LOADED = 'all_loaded'
+
+LOADED = 'loaded'
+
+TOTAL = 'total'
+
 
 def _safe_extended_peer_info(ext_peer_info):
     """
@@ -221,9 +227,9 @@ class DownloadsEndpoint(RESTEndpoint):
                         'time_added': Integer
                     }),
                     'checkpoints': schema(Checkpoints={
-                        'total': Integer,
-                        'loaded': Integer,
-                        'all_loaded': Boolean,
+                        TOTAL: Integer,
+                        LOADED: Integer,
+                        ALL_LOADED: Boolean,
                     })
                 }),
             }
@@ -243,9 +249,9 @@ class DownloadsEndpoint(RESTEndpoint):
         get_files = params.get('get_files', '0') == '1'
 
         checkpoints = {
-            "total": self.download_manager.checkpoints_count,
-            "loaded": self.download_manager.checkpoints_loaded,
-            "all_loaded": self.download_manager.all_checkpoints_are_loaded,
+            TOTAL: self.download_manager.checkpoints_count,
+            LOADED: self.download_manager.checkpoints_loaded,
+            ALL_LOADED: self.download_manager.all_checkpoints_are_loaded,
         }
 
         if not self.download_manager.all_checkpoints_are_loaded:

[kozlovsky]

I understand, and I'm questioning the benefit of it. If applying this approach consistently, we will have around 50 similar constants, 50 more lines of code, and then we need to use these constants on the GUI side to make them really useful. And to use them on the GUI side, we need to extract them to a separate module.

The usual "rule of three" of refactoring assumes that "two instances of similar code do not require refactoring, but when similar code is used three times, it should be extracted". If we only replace the constant in two places, the cost/benefit ratio is usually not favorable for refactoring.

I prefer later switch to Pydantic and FastAPI and avoid explicit Swagger definitions altogether.

[drew2a]

And to use them on the GUI side, we need to extract them to a separate module.

No, we can use constants from CORE in the GUI, as we already doing now. But I don't encourage you to do that.

The usual "rule of three" of refactoring assumes that "two instances of similar code do not require refactoring, but when similar code is used three times, it should be extracted".

I disagree with this approach: to use the rule of three everywhere. It is useful when you do refactoring of a complex code and it is hard to say whether it is beneficial to extract some common logic or not (will you add unnecessary complexity or not?).

But this case is different. We talking about extracting the string that was used twice in the same file and in the same PR. Copy-paste is a straightforward and easy-to-apply approach, but it may lead to problems in the future.

As a reviewer, I should check whether you wrote it correctly or made a mistake. I believe that you copy the value and past a value, so in this case, the chance of doing a mistake is minimal (but not zero).

But in the future, during a hotfix or refactoring it may happen that in one place the value will be changed and in the other not. However, this risk is easily eliminated. If you do only one action, press alt+ctrl+c and extract the string to a constant, the case that I described above will be impossible.

So, if I choose from two options:

1. To write a code with the probability of mistyping now or in the future
2. To write a code without the probability of mistyping now or in the future
   then I would choose the second option.

That is why I suggest doing this in the current PR.

I prefer later switch to Pydantic and FastAPI and avoid explicit Swagger definitions altogether.

Agree, it would be nice. But it also may not happen.

[kozlovsky]

I added the constants that you requested. I still think it makes the code slightly worse, but it is not an important enough issue to start bikeshedding about it :)

[drew2a]

Thank you for addressing all the comments 🙏 .