Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(#27): address ambiguity for score fields, allow assign the priority manually #29

Merged
merged 21 commits into from
Jun 30, 2023

Conversation

lucasoares
Copy link
Member

@lucasoares lucasoares commented Jun 28, 2023

Closes #27

This PR makes many package modifications and field naming to prevent ambiguity.

It also changes a few internal definitions. Previously the "max score" definition for our codebase was 0 but as discussed in #27 we will keep the definition where score means the priority value, not the priority itself:

  • A lower score means higher priority and the lowest score accepted by Deckard is 0

  • A higher score means lower priority, and the higher score accepted by Deckard is 9007199254740992

  • Allow users to assign priority while adding message

    • If the score is not set, the value will be set with the current timestamp in milliseconds at the moment of the message creation.
    • Negative scores will be converted to 0, adding the message with the lowest score (and highest priority)
  • Add max_score and min_score score filtering for Pull requests

    • score_filter is now deprecated and should not be used. Prefer using max_score now but remember that it behaves differently: The max_score field is the upper threshold itself, but the score_filter will result in an upper score threshold of the current timestamp minus the score_filter value.
    • max_score: Sets the upper threshold for the priority score of a message to be returned in the pull request.
      • Only messages with a priority score equal to or lower than the max_score value will be returned.
      • The maximum score accepted by Deckard is 9007199254740992, any value higher than this will be capped to the maximum score.
      • To set this value to the minimum score accepted by Deckard, use any negative number.
      • This parameter will be ignored if set to 0 (default value) and the Pull request will not have an upper threshold for filtering scores (the upper threshold will be the maximum score).
    • min_score: Sets the lower threshold for the priority score required for a message to be returned.
    • Only messages with a priority score equal to or higher than the min_score value will be returned.
    • The minimum score accepted by Deckard is 0 which is also the default value
  • Allow users manually set the score field on Ack requests

    • If used at the same time with the 'lock_ms' attribute, the message will be locked for the specified time and then returned to the queue with the specified score.
    • For ACK requests, if the score is not provided (or set to 0), the message will return to the queue with the default score algorithm which is the current timestamp in milliseconds.
    • For NACKs requests, if the score is not provided (or set to 0), the message will return to the queue with the minimum score accepted by Deckard which is 0.
    • Negative values will be converted to 0, which is how to set the highest priority to a message in an ACK/NACK request.
  • increase_score and decrease_score for Ack requests

Breaking change

I changed the ZREVRANGEBYSCORE and ZRANGEBYSCORE to ZRANGE. I think if I will change this back so Deckard can work with older Redis Versions. ZRANGE additional attributes are only available on 6.2.0+: https://redis.io/commands/zrange/

  • - Check if this must be changed back - changed back
  • - Create a breaking-changes automatic test, to make sure clients with old APIs will be able to use the new Deckard version - I made manual compatibility tests (running example in both client and server versions) and later I will create a automatic breaking changes test

Sorry, something went wrong.

@codecov
Copy link

codecov bot commented Jun 28, 2023

Codecov Report

Merging #29 (ff62c85) into main (75a1933) will increase coverage by 0.39%.
The diff coverage is 92.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #29      +/-   ##
==========================================
+ Coverage   81.45%   81.85%   +0.39%     
==========================================
  Files          24       26       +2     
  Lines        3451     3571     +120     
==========================================
+ Hits         2811     2923     +112     
+ Misses        515      513       -2     
- Partials      125      135      +10     
Impacted Files Coverage Δ
internal/cmd/deckard/main.go 50.00% <0.00%> (ø)
internal/queue/cache/cache.go 0.00% <ø> (ø)
internal/queue/storage/storage.go 0.00% <ø> (ø)
internal/queue/utils/primitive_conversion.go 100.00% <ø> (ø)
internal/queue/queue_housekeeper.go 79.34% <81.48%> (-1.04%) ⬇️
internal/service/deckard_service.go 78.22% <81.81%> (-1.42%) ⬇️
internal/queue/storage/mongo_storage.go 78.93% <93.93%> (ø)
internal/queue/cache/memory_cache.go 89.52% <96.77%> (-0.63%) ⬇️
internal/queue/queue.go 89.68% <98.00%> (-0.10%) ⬇️
internal/audit/audit.go 86.88% <100.00%> (ø)
... and 6 more

@lucasoares lucasoares marked this pull request as draft June 28, 2023 21:14
@lucasoares lucasoares marked this pull request as ready for review June 29, 2023 21:45
internal/dtime/deckard_time.go Show resolved Hide resolved
internal/queue/cache/memory_cache.go Outdated Show resolved Hide resolved
@cezar-tech cezar-tech self-requested a review June 30, 2023 12:39
lucasoares and others added 5 commits June 30, 2023 11:49
@lucasoares lucasoares merged commit 3301bde into main Jun 30, 2023
@lucasoares lucasoares deleted the set-priority-27 branch June 30, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address ambiguity for priority/score field names and additional priority score features
2 participants