-
Notifications
You must be signed in to change notification settings - Fork 130
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
Split redis #1643
Split redis #1643
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1643 +/- ##
==========================================
- Coverage 77.83% 77.83% -0.01%
==========================================
Files 230 232 +2
Lines 18181 18209 +28
==========================================
+ Hits 14152 14173 +21
- Misses 4029 4036 +7
Continue to review full report at Codecov.
|
closes #1627 |
Failed playwright test I think is just flaky. Works for me locally. I believe this is ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tons of cleanup in here, nice!
@@ -5,4 +5,6 @@ FROM ${COSMOS_REGISTRY}/redis:6.2 | |||
RUN mkdir /config | |||
COPY ./config/* /config/ | |||
|
|||
EXPOSE 3680 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Expose controls what is shown when you do "docker ps" or whatever. Unfortunately you can't remove an earlier EXPOSE, so I just added 3680 to the container so both 3679 and 3680 would show.
@@ -0,0 +1,4 @@ | |||
appendonly no | |||
save "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this disable saving the state?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this fully turns off persistence.
@@ -0,0 +1,4 @@ | |||
appendonly no | |||
save "" | |||
port 6380 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The existing redis.conf doesn't have a port specified ... I assume its the default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6379 is the default
end | ||
# Return the results sorted by target, packet | ||
result.sort_by { |a| [a[0], a[1]] } | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
get_all_cmd_tlm_info wasn't used anywhere. _get_cnt I moved to Topic.
require 'cosmos/models/model' | ||
|
||
module Cosmos | ||
class SettingsModel < Model |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow we didn't already have this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently got a little lazy with the models. Store should be completely isolated to Models and Topic now.
|
||
def self.read_topics(topics, offsets = nil, timeout_ms = 1000, &block) | ||
Store.read_topics(topics, offsets, timeout_ms, &block) | ||
if RUBY_VERSION < "3" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this just < "3"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It works to check for all 2.x versions
Looks like the playwright failed on the calendar test. That one is a little flaky so I wouldn't worry too much about it. |
closes #1636