Skip to content

Conversation

@vdusek
Copy link
Contributor

@vdusek vdusek commented Oct 22, 2025

I didn't have an opportunity to provide feedback on #573 as it was merged while I was on vacation. While the implementation works well and the costs and performance improvements in single mode are great (good job @Pijukatel), I have concerns about the current architecture...

IMO, we already have quite a lot of storage client classes: base classes with docstrings, the "storage client openers" for each backend type, and the individual storage clients for all storage types. By introducing the split into single and shared modes, we've added yet another layer to the Apify RQ storage client. Also, it implements the RQ storage client base class interface, but only partially. Altogether, this negatively affects readability, maintainability, and debuggability (I ran into this while investigating the truncated unique keys issue), and it increases code duplication (e.g., identical docstrings repeated in multiple places).

This PR merges the three request queue client classes (ApifyRequestQueueClient, ApifyRequestQueueSingleClient, ApifyRequestQueueSharedClient) into a single ApifyRequestQueueClient class. Methods now use conditional logic based on the access parameter to determine whether to call _method_single() or _method_shared() variants.

This change reduces the total lines of code, removes one layer of storage clients, eliminates docstring duplication, and all existing tests are passing.

Let me know what you think.

@vdusek vdusek added this to the 126th sprint - Tooling team milestone Oct 22, 2025
@vdusek vdusek requested review from Pijukatel and janbuchar October 22, 2025 14:45
@vdusek vdusek self-assigned this Oct 22, 2025
@vdusek vdusek added adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team. labels Oct 22, 2025
@janbuchar janbuchar removed their request for review October 22, 2025 14:57
@janbuchar
Copy link
Contributor

I'll leave the review to @Pijukatel as the author of the current thing. My two cents are that I like the simplicity of a single class, but IIRC there are some instance attributes that are only accessed by one of the two implementations, which makes thing kinda awkward.

@Pijukatel
Copy link
Contributor

I also considered this approach initially, but I did not think it was a good one for the following reasons:

  • The "big class" contains methods and attributes that no single instance will fully use, which needlessly clutters it,
  • While being one class, I do not think that it improves readability, because when reading it, you have to remember one more value in your head because of this setup. Sure, one extra value seems trivial, but it compounds with whatever problem you are dealing with. (If you are not sure what I mean by this, maybe check Chunking and Aliasing part of this talk )
  • The distinction between the shared code and unique code is inbuilt into the naming, which I think is a less clear distinction than using a different class for it. So instead of having one file that contains all methods of one implementation in one place, you have to filter them in your head when reading the big file.
  • I feel like the class is borderline breaking the Single Responsibility principle, as it contains two distinct implementations of the problem.

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

Labels

adhoc Ad-hoc unplanned task added during the sprint. t-tooling Issues with this label are in the ownership of the tooling team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants