-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[v2][storage] Add dependency store to v2 storage interface #6297
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
Merged
Merged
Changes from 8 commits
Commits
Show all changes
21 commits
Select commit
Hold shift + click to select a range
20c9702
Add Interface For Dependency Store Reader
mahadzaryab1 8c00ee1
Add package_test.go
mahadzaryab1 2fb8fc8
Create Separate Factory For Dependency Store
mahadzaryab1 1ce9ffe
Change Return Type of Factory Adapter And Implement Depstore Factory
mahadzaryab1 81010bf
Add Unit Tests
mahadzaryab1 7aae69b
Add License
mahadzaryab1 934ca7a
Change Return Type of GetStorageFactoryV2
mahadzaryab1 a5e470d
Merge branch 'main' into v2-depreader
mahadzaryab1 daae5b9
Change Reader Interface To Take Struct Of Parameters
mahadzaryab1 424d845
Implement Dependency Reader Wrapper
mahadzaryab1 9ce6576
Update Factory To Use New Reader Wrapper
mahadzaryab1 e395f52
Merge branch 'main' into v2-depreader
mahadzaryab1 d07eebc
Generate Mocks For New Interface
mahadzaryab1 aa112ed
Use V2 Factory To Get Dependency Reader
mahadzaryab1 bcdc86d
Fix Tests
mahadzaryab1 55d3d19
Fix Tests
mahadzaryab1 f43ec56
Create Separate Accessors For Trace And Dependency Stores
mahadzaryab1 4254ab4
Merge branch 'main' into v2-depreader
mahadzaryab1 38d0807
Merge branch 'main' into v2-depreader
mahadzaryab1 49de2c2
Get Dependency Factory From Runtime Check
mahadzaryab1 47d5f2c
Remove Base Factory
mahadzaryab1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| // Copyright (c) 2024 The Jaeger Authors. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package depstore | ||
|
|
||
| import "github.com/jaegertracing/jaeger/storage_v2" | ||
|
|
||
| type Factory interface { | ||
| storage_v2.FactoryBase | ||
yurishkuro marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| CreateDependencyReader() (Reader, error) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| // Copyright (c) 2024 The Jaeger Authors. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package depstore | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/jaegertracing/jaeger/pkg/testutils" | ||
| ) | ||
|
|
||
| func TestMain(m *testing.M) { | ||
| testutils.VerifyGoLeaks(m) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // Copyright (c) 2024 The Jaeger Authors. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package depstore | ||
|
|
||
| import ( | ||
| "context" | ||
| "time" | ||
|
|
||
| "github.com/jaegertracing/jaeger/model" | ||
| ) | ||
|
|
||
| // Reader can load service dependencies from storage. | ||
| type Reader interface { | ||
| GetDependencies(ctx context.Context, endTs time.Time, lookback time.Duration) ([]model.DependencyLink, error) | ||
yurishkuro marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
don't know about this - adapter is implementation detail, this function should not leak it
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.
@yurishkuro I was trying to follow's Go's best practice here of returning structs instead of interfaces. If we want to keep the interface return, we'll need to define another function
GetDependencyStoreFactorywith a different return type (depstore.Reader) but it will do the exact same thing. Let me know what you think.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's not the best practice in this case. The return-struct best practice applies to constructors.
GetStorageFactoryV2is not a constructor, it's an accessor that is supposed to return a storage interface. The caller of this function is not supposed to know about the implementation of that interface and accordingly should not be able to take dependency on the concrete returned type, which could be wider than the interface alone.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.
Sounds good - so should we have two accessors here (something like
GetTraceStoreFactoryandGetDependencyStoreFactory)? Their return types would be different but the implementations would be the same.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, we should. Implementations are only the same today, not in the future.
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.
sounds good to me! done