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

8.0.1 Time on locks & changelog UI is incorrect/wrong #761

Closed
JeremiahChurch opened this issue Mar 9, 2023 · 5 comments · Fixed by #804
Closed

8.0.1 Time on locks & changelog UI is incorrect/wrong #761

JeremiahChurch opened this issue Mar 9, 2023 · 5 comments · Fixed by #804

Comments

@JeremiahChurch
Copy link
Contributor

JeremiahChurch commented Mar 9, 2023

Describe the bug
took the 8.0.1/sidekiq 7 upgrade to try and resolve some non-releasing-lockd issues.

The locks screen is showing 1970-01-01 values in the 'since' column and the changelogs screen is showing 'bogus' for time. looks like a date integer coercion issue? Also appears to be creating empty lock values? I'm digging through the changes to see if I can find it...

the locks detail screen does show the correct time.

I cleared the locks and changeset (via the methods the UI calls) in case the upgrade from unique-jobs 7.x had problems - issue remains

image

image

image

a dump of a few of the values
SidekiqUniqueJobs::Digests.new.page(cursor:100, pattern: '*', page_size: 25)

[Lock status for prod_unique_jobs:3fdab3c234fd02f80a1da7f37d6dc9dc

          value: ffb803c6d0bf71fa81f5c10d
           info: {"worker"=>"DiscussionThreads::UpdateDiscussionsReadJob", "queue"=>"webhook_sync", "limit"=>nil, "timeout"=>0, "ttl"=>2109, "type"=>"until_executed", "lock_args"=>[17151205, 6], "time"=>1678370128.136117}
    queued_jids: []
    primed_jids: []
    locked_jids: ["ffb803c6d0bf71fa81f5c10d"]
     changelogs: []
,
  Lock status for 1678370128.1982968

          value: 
           info: 
    queued_jids: []
    primed_jids: []
    locked_jids: []
     changelogs: []
,
  Lock status for prod_unique_jobs:e46aff0823ab33ec7e0208af451188a7

          value: b936b6065e0431af2c9fa059
           info: {"worker"=>"DiscussionThreads::UpdateDiscussionsReadJob", "queue"=>"webhook_sync", "limit"=>nil, "timeout"=>0, "ttl"=>1922, "type"=>"until_executed", "lock_args"=>[12455593, 3], "time"=>1678370352.083641}
    queued_jids: []
    primed_jids: []
    locked_jids: ["b936b6065e0431af2c9fa059"]
     changelogs: []
,
  Lock status for 1678370352.1326859

          value: 
           info: 
    queued_jids: []
    primed_jids: []
    locked_jids: []
     changelogs: []
,
  Lock status for prod_unique_jobs:41b36112361e536032784f2c9096655f

          value: 99fd4a3c921e69e7f38235d0
           info: {"worker"=>"DiscussionThreads::UpdateDiscussionsReadJob", "queue"=>"webhook_sync", "limit"=>nil, "timeout"=>0, "ttl"=>2169, "type"=>"until_executed", "lock_args"=>[17856623, 7], "time"=>1678370243.577242}
    queued_jids: []
    primed_jids: []
    locked_jids: ["99fd4a3c921e69e7f38235d0"]

Expected behavior
correct dates

Current behavior
incorrect dates and extra locks?

Additional context
sidekiq 7.0.6
sidekiq-unique-jobs 8.0.1
sidekiq-scheduler 5.0.2

@JeremiahChurch JeremiahChurch changed the title Time on locks & changelog UI is incorrect/wrong 8.0.1 Time on locks & changelog UI is incorrect/wrong Mar 9, 2023
@JeremiahChurch
Copy link
Contributor Author

JeremiahChurch commented Mar 9, 2023

It looks like the result of digests[1].map in Digests#page has changed because the underlying values are stored/returned differently and that is causing the multiple rows to be returned - the 'blank' is just the time for the lock

    def page(cursor: 0, pattern: SCAN_PATTERN, page_size: 100)
      redis do |conn|
        total_size, digests = conn.multi do |pipeline|
          pipeline.zcard(key)
          pipeline.zscan(key, cursor, match: pattern, count: page_size)
        end
        puts "\n\ndigests: #{digests[1].inspect}\n\n"
        # NOTE: When debugging, check the last item in the returned array.
        [
          total_size.to_i,
          digests[0].to_i, # next_cursor
          digests[1].map { |digest, score| Lock.new(digest, time: score) }, # entries
        ]
      end

returns

digests: ["0", 
["prod_unique_jobs:2e3f377528bd7e5dafca80f993144d81", "1678393091.0208826", 
"prod_unique_jobs:94f085827423f76b1f3e610bf316ad1c", "1678393399.1548831"]
]

when instead it should be nested more like:

digests: ["0", 
[
["prod_unique_jobs:2e3f377528bd7e5dafca80f993144d81", "1678393091.0208826"], 
["prod_unique_jobs:94f085827423f76b1f3e610bf316ad1c", "1678393399.1548831"]
]
]

a monkey patch to use activesupport's in_groups_of(2) gets the page looking good without my changes in #762

module SidekiqUniqueJobs
  class Digests
    def page(cursor: 0, pattern: SCAN_PATTERN, page_size: 100)
      redis do |conn|
        total_size, digests = conn.multi do |pipeline|
          pipeline.zcard(key)
          pipeline.zscan(key, cursor, match: pattern, count: page_size)
        end

        # NOTE: When debugging, check the last item in the returned array.
        [
          total_size.to_i,
          digests[0].to_i, # next_cursor
          digests[1].in_groups_of(2).map { |digest, score| Lock.new(digest, time: score) }, # entries
        ]
      end
    end
  end
end

JeremiahChurch added a commit to JeremiahChurch/sidekiq-unique-jobs that referenced this issue Mar 23, 2023
@mhenrixon
Copy link
Owner

Can I close this then @JeremiahChurch?

@JeremiahChurch
Copy link
Contributor Author

Can I close this then @JeremiahChurch?

@mhenrixon without that monkey patch the locks page is still broken. I'm assuming it should generally be fixed?

As-is on 8.0.2 without that patch you'll see

image

with the monkey patch the 'extra' lines are gone

image

If you're alright with active support as a depend I'll pull a PR together if you'd like?

@Earlopain
Copy link
Contributor

@JeremiahChurch I don't think the dependency on active_support is needed. each_slice should do the job just as well.

@mhenrixon
Copy link
Owner

@JeremiahChurch I don't think the dependency on active_support is needed. each_slice should do the job just as well.

Great suggestion, I'll have a look at that.

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 a pull request may close this issue.

3 participants