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

V2 feature/add queue manager #116

Merged
merged 4 commits into from
May 6, 2021
Merged

V2 feature/add queue manager #116

merged 4 commits into from
May 6, 2021

Conversation

liguangbo
Copy link
Collaborator

close #115

@liguangbo liguangbo added this to the v2 milestone Apr 26, 2021
Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall was LGTM, except I'm not sure the name of MetaManager was suitable while it's too generic and not easy to fast identify what meta indeed. maybe a name like QueueManager was better than that, how do you think.

return nil
}

func (m *MetaManager) publish() error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems notify would be more precious than publish since its duty was to notify the lmstfy server to pull the new configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix

@codecov
Copy link

codecov bot commented Apr 27, 2021

Codecov Report

Merging #116 (eb100cd) into v2 (5d3af6d) will increase coverage by 0.63%.
The diff coverage is 63.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##               v2     #116      +/-   ##
==========================================
+ Coverage   51.02%   51.66%   +0.63%     
==========================================
  Files          24       26       +2     
  Lines        1858     1955      +97     
==========================================
+ Hits          948     1010      +62     
- Misses        777      797      +20     
- Partials      133      148      +15     
Flag Coverage Δ
unittests 51.66% <63.91%> (+0.63%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
engine/redis-v2/utils.go 50.00% <50.00%> (ø)
engine/redis-v2/queue_manager.go 64.21% <64.21%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d3af6d...eb100cd. Read the comment docs.

@liguangbo
Copy link
Collaborator Author

overall was LGTM, except I'm not sure the name of MetaManager was suitable while it's too generic and not easy to fast identify what meta indeed. maybe a name like QueueManager was better than that, how do you think.

refactor to QueueManager, but feel something strange. @chensunny what do you think?

@liguangbo liguangbo changed the title V2 feature/add metadata manager V2 feature/add queue manager Apr 27, 2021
@git-hulk
Copy link
Member

overall was LGTM, except I'm not sure the name of MetaManager was suitable while it's too generic and not easy to fast identify what meta indeed. maybe a name like QueueManager was better than that, how do you think.

refactor to QueueManager, but feel something strange. @chensunny what do you think?

ahh... It's hard to get a right name

@liguangbo liguangbo merged commit 670dd12 into v2 May 6, 2021
@liguangbo liguangbo deleted the v2-feature/metadata-manager branch May 7, 2021 07:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants