fix: 如果获取不到config，尝试远端同步获取一次，获取到的内容需要更新缓存 #301
Conversation
dosubot
bot
added
the
size:M
WalkthroughThe updates to the client code encompass improvements in documentation, error handling, and caching mechanisms in Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
感谢您提出Pull Request，我会尽快Review。我会在1-2日内进行查看或者回复，如果遇到节假日可能会处理较慢，敬请谅解。 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- client.go (5 hunks)
Additional comments: 7
client.go (7)
- 63-63: The comment for the
Clientinterface has been updated to include the Chinese description "apollo 客户端接口". While it's helpful to have comments in multiple languages for a global audience, it's important to ensure consistency across the codebase. If multilingual comments are a standard practice in this project, this change is appropriate. Otherwise, consider sticking to a single language (preferably English for broader accessibility) for comments and documentation.- 150-150: The
GetConfigmethod now directly callsGetConfigAndInit, effectively making these two methods identical in functionality. This change simplifies the retrieval process by ensuring that initialization is always attempted when fetching a configuration. However, it's crucial to ensure that this does not introduce unnecessary overhead or duplicate initialization efforts, especially ifGetConfigAndInitis called multiple times for the same namespace. Consider adding a mechanism to avoid redundant initialization if this could be a concern.- 165-182: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [155-171]
The
GetConfigAndInitmethod has been updated to include logic for initializing the cache and synchronizing configuration with a namespace if the initial cache lookup fails. This directly addresses the PR's objective by ensuring that the local cache is updated with configuration details fetched from a remote source. However, there are a few points to consider:
- Ensure that
syncApolloConfig.SyncWithNamespaceis thread-safe, as concurrent calls toGetConfigAndInitcould lead to race conditions.- It might be beneficial to log the outcome of the synchronization attempt (success or failure) for debugging and monitoring purposes.
- Consider handling the case where
apolloConfigis nil more explicitly, perhaps by logging a warning or error, to aid in troubleshooting scenarios where configuration synchronization fails.
- 179-179: The
GetConfigCachemethod's implementation remains unchanged, but it's worth noting that it relies onGetConfigAndInitto ensure the cache is initialized and up-to-date. This reinforces the importance of the changes made toGetConfigAndInitand highlights the interconnectedness of these methods. Ensure that the behavior ofGetConfigAndInitis thoroughly tested, especially in edge cases where the configuration might fail to initialize or synchronize.- 189-189: The
GetDefaultConfigCachemethod now explicitly fetches the default namespace's configuration and its cache. This change is straightforward and aligns with the PR's objectives by ensuring that even the default configuration cache is subject to the same initialization and synchronization logic as other namespaces. No specific issues or concerns here, but it's another point where the robustness ofGetConfigAndInitis crucial.- 198-198: The
GetApolloConfigCachemethod is a simple alias forGetDefaultConfigCache. This change seems to be part of the effort to improve naming conventions and clarity in the API. It's a good practice to have clear and descriptive method names, but ensure that this renaming does not cause confusion or break existing code that may rely on the old method names. Consider adding deprecation notices to any renamed methods if backward compatibility is a concern.- 203-233: The methods
GetValue,GetStringValue,GetIntValue,GetFloatValue,GetBoolValue,GetStringSliceValue, andGetIntSliceValuehave been updated to fetch values from the default namespace's configuration. These changes are part of the naming convention and functionality enhancements mentioned in the PR summary. While the changes themselves are straightforward, it's important to ensure that they are consistently applied across the entire API and that any potential impact on existing code is carefully considered. Additionally, the use of a separator for slice values (GetStringSliceValueandGetIntSliceValue) should be documented clearly if it's a new or changed behavior.
Pull Request Test Coverage Report for Build 8166509762Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
This looks like a bug that the local cache is not updated. Would you please help to write a unit test so that this logic could be covered? |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
go.modis excluded by:!**/*.mod
Files selected for processing (2)
- client.go (5 hunks)
- client_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- client.go
Additional comments: 4
client_test.go (4)
- 25-25: The addition of the
reflectpackage import is directly related to the use of reflection in the new test functions for mocking purposes.- 29-30: The import of
github.com/agiledragon/gomonkey/v2is necessary for the patching functionality used in the new test functions to mock external dependencies. This approach allows for more controlled testing environments.- 32-32: The addition of
github.com/apolloconfig/agollo/v4/component/remoteis required for the typeAbsApolloConfigused in the new test functions. This ensures that the tests can accurately simulate interactions with remote configuration sources.- 44-44: The modification to the
initfunction does not introduce any new logic but is included for context. It's essential to ensure that any changes in the initialization process do not inadvertently affect the behavior of the new tests.
client_test.go
Outdated
| func TestGetConfigAndInitValNotNil(t *testing.T) { | ||
| var apc *remote.AbsApolloConfig | ||
| patch := gomonkey.ApplyMethod(reflect.TypeOf(apc), "SyncWithNamespace", func(_ *remote.AbsApolloConfig, namespace string, appConfigFunc func() config.AppConfig) *config.ApolloConfig { | ||
| return &config.ApolloConfig{ | ||
| ApolloConnConfig: config.ApolloConnConfig{ | ||
| AppID: "testID", | ||
| NamespaceName: "testNotFound", | ||
| }, | ||
| Configurations: map[string]interface{}{"testKey": "testValue"}, | ||
| } | ||
| }) | ||
| defer patch.Reset() | ||
|
|
||
| client := createMockApolloConfig(120) | ||
| cf := client.GetConfig("testNotFound") | ||
| Assert(t, cf, NotNilVal()) | ||
| // cache should be updated | ||
| Assert(t, client.cache.GetConfig("testNotFound"), NotNilVal()) | ||
| } |
There was a problem hiding this comment.
The test TestGetConfigAndInitValNotNil uses the gomonkey package to mock the SyncWithNamespace method of AbsApolloConfig. This test aims to verify that when a valid configuration is returned by the mocked method, the cache is updated accordingly. This directly addresses the PR's objective to ensure the cache is updated with remotely fetched content when local retrieval fails.
However, it's crucial to ensure that the test accurately reflects the conditions under which the cache should be updated. The test should also verify that the cache contains the expected configuration values, not just that it is non-nil.
Consider enhancing the test to assert that the cache contains the expected configuration values after the SyncWithNamespace method is called. This will provide a more thorough validation of the cache update mechanism.
+ Assert(t, client.cache.GetConfig("testNotFound").Configurations["testKey"], Equal("testValue"))Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func TestGetConfigAndInitValNotNil(t *testing.T) { | |
| var apc *remote.AbsApolloConfig | |
| patch := gomonkey.ApplyMethod(reflect.TypeOf(apc), "SyncWithNamespace", func(_ *remote.AbsApolloConfig, namespace string, appConfigFunc func() config.AppConfig) *config.ApolloConfig { | |
| return &config.ApolloConfig{ | |
| ApolloConnConfig: config.ApolloConnConfig{ | |
| AppID: "testID", | |
| NamespaceName: "testNotFound", | |
| }, | |
| Configurations: map[string]interface{}{"testKey": "testValue"}, | |
| } | |
| }) | |
| defer patch.Reset() | |
| client := createMockApolloConfig(120) | |
| cf := client.GetConfig("testNotFound") | |
| Assert(t, cf, NotNilVal()) | |
| // cache should be updated | |
| Assert(t, client.cache.GetConfig("testNotFound"), NotNilVal()) | |
| } | |
| func TestGetConfigAndInitValNotNil(t *testing.T) { | |
| var apc *remote.AbsApolloConfig | |
| patch := gomonkey.ApplyMethod(reflect.TypeOf(apc), "SyncWithNamespace", func(_ *remote.AbsApolloConfig, namespace string, appConfigFunc func() config.AppConfig) *config.ApolloConfig { | |
| return &config.ApolloConfig{ | |
| ApolloConnConfig: config.ApolloConnConfig{ | |
| AppID: "testID", | |
| NamespaceName: "testNotFound", | |
| }, | |
| Configurations: map[string]interface{}{"testKey": "testValue"}, | |
| } | |
| }) | |
| defer patch.Reset() | |
| client := createMockApolloConfig(120) | |
| cf := client.GetConfig("testNotFound") | |
| Assert(t, cf, NotNilVal()) | |
| // cache should be updated | |
| Assert(t, client.cache.GetConfig("testNotFound"), NotNilVal()) | |
| Assert(t, client.cache.GetConfig("testNotFound").Configurations["testKey"], Equal("testValue")) | |
| } |
client_test.go
Outdated
| func TestGetConfigAndInitValNil(t *testing.T) { | ||
| var apc *remote.AbsApolloConfig | ||
| patch := gomonkey.ApplyMethod(reflect.TypeOf(apc), "SyncWithNamespace", func(_ *remote.AbsApolloConfig, namespace string, appConfigFunc func() config.AppConfig) *config.ApolloConfig { | ||
| return nil | ||
| }) | ||
| defer patch.Reset() | ||
|
|
||
| client := createMockApolloConfig(120) | ||
| cf := client.GetConfig("testNotFound") | ||
| Assert(t, cf, NilVal()) | ||
| } |
There was a problem hiding this comment.
The test TestGetConfigAndInitValNil similarly uses the gomonkey package to mock the SyncWithNamespace method of AbsApolloConfig, but in this case, it simulates a scenario where the method returns nil. This test aims to verify that the configuration fetched is nil when the remote source does not provide a valid configuration.
This test is important for ensuring that the system behaves predictably in scenarios where the remote configuration source is unavailable or returns an invalid response. However, it would be beneficial to also verify that the cache is not updated in this scenario, which is not currently covered by the test.
Add an assertion to verify that the cache is not updated when SyncWithNamespace returns nil. This will ensure that the system's behavior aligns with expectations in error scenarios.
+ Assert(t, client.cache.GetConfig("testNotFound"), NilVal())Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| func TestGetConfigAndInitValNil(t *testing.T) { | |
| var apc *remote.AbsApolloConfig | |
| patch := gomonkey.ApplyMethod(reflect.TypeOf(apc), "SyncWithNamespace", func(_ *remote.AbsApolloConfig, namespace string, appConfigFunc func() config.AppConfig) *config.ApolloConfig { | |
| return nil | |
| }) | |
| defer patch.Reset() | |
| client := createMockApolloConfig(120) | |
| cf := client.GetConfig("testNotFound") | |
| Assert(t, cf, NilVal()) | |
| } | |
| func TestGetConfigAndInitValNil(t *testing.T) { | |
| var apc *remote.AbsApolloConfig | |
| patch := gomonkey.ApplyMethod(reflect.TypeOf(apc), "SyncWithNamespace", func(_ *remote.AbsApolloConfig, namespace string, appConfigFunc func() config.AppConfig) *config.ApolloConfig { | |
| return nil | |
| }) | |
| defer patch.Reset() | |
| client := createMockApolloConfig(120) | |
| cf := client.GetConfig("testNotFound") | |
| Assert(t, cf, NilVal()) | |
| Assert(t, client.cache.GetConfig("testNotFound"), NilVal()) | |
| } |
|
增加：去除同方法内 cache init 步骤，同理没有赋值并没有意义，且在 UpdateApolloConfig 步骤中，会有 cache store 的步骤 |
在获取不到 config 配置的时候，没有将远端获取的内容更新到本地缓存，让查询变得无意义
Summary by CodeRabbit
gomonkeyfor mocking.