-
Notifications
You must be signed in to change notification settings - Fork 318
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
Cache integrations - service naming #3056
Conversation
Overall package sizeSelf size: 4.15 MB Dependency sizes
🤖 This report was automatically generated by heaviest-objects-in-the-universe |
Codecov Report
@@ Coverage Diff @@
## master #3056 +/- ##
==========================================
- Coverage 87.62% 86.65% -0.97%
==========================================
Files 297 333 +36
Lines 10994 11866 +872
Branches 33 33
==========================================
+ Hits 9633 10282 +649
- Misses 1361 1584 +223
... and 32 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
392df87
to
aeeefde
Compare
@@ -4,6 +4,13 @@ const StoragePlugin = require('./storage') | |||
|
|||
class CachePlugin extends StoragePlugin { | |||
static get operation () { return 'command' } | |||
|
|||
startSpan (options) { |
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.
I think this part might be a little confusing. Looking across the caching/mysql/tedious PRs: In some classes, startSpan takes two arguments, but in cache related classes, it only takes one?
Or, is the change to make it a single argument going to happen everywhere? There are so many PRs I'm losing track of some of this stuff 😅
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.
Cache plugins get this behaviour generalized because they're all upgraded to respond to the naming schema, so it's possible to do so. Database plugins aren't all yet upgraded, so we can't yet generalize for them, but since it's helping reduce verbosity in the individual plugins I see it as something we want to do when we can.
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.
Maybe as a stop-gap we can rename this method to startSpanImpliedOperation(options)
and have it still call super.startSpan
? With a TODO to refactor startSpan
everywhere to have the same arity?
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.
That sounds better for continuity, will do.
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.
That name doesn't really help with understanding IMO. Also, the point of the startSpan
method is explicitly to allow overriding it in every plugin.
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.
naming lgtm
e825de4
to
3bde47b
Compare
3bde47b
to
ccada49
Compare
Adopt service naming schema in cache integrations --------- Co-authored-by: Thomas Hunter II <[email protected]>
Adopt service naming schema in cache integrations --------- Co-authored-by: Thomas Hunter II <[email protected]>
Adopt service naming schema in cache integrations --------- Co-authored-by: Thomas Hunter II <[email protected]>
Adopt service naming schema in cache integrations --------- Co-authored-by: Thomas Hunter II <[email protected]>
Adopt service naming schema in cache integrations --------- Co-authored-by: Thomas Hunter II <[email protected]>
Adopt service naming schema in cache integrations --------- Co-authored-by: Thomas Hunter II <[email protected]>
Adopt service naming schema in cache integrations --------- Co-authored-by: Thomas Hunter II <[email protected]>
What does this PR do?
This applies the service naming schema logic (see #2941, #2961) to cache integrations.
These are more complex than messaging integrations, as we need to take into account the default provided by DatabasePlugin into the naming functions, which version 0 must respect, but version 1 does not need.
Motivation
Keep trucking on naming schema adoption.
Plugin Checklist
Additional Notes