【WIP】增加一个根据 DataID 清理 Data 上的 Publisher 数据，以及忽略客户端发送 Publisher 的能力 #361
Conversation
hui-cha
changed the title
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.8.0)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.javaAn unexpected error occurred while running PMD. server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.javaAn unexpected error occurred while running PMD. server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/filter/LeaderForwardFilter.javaAn unexpected error occurred while running PMD.
WalkthroughThe pull request introduces a comprehensive feature for managing a blacklist of data information IDs in the SOFA Registry system. This functionality allows administrators to dynamically control and block specific data IDs across session and meta servers. The implementation includes a new constant, a RESTful resource for blacklist management, services for fetching and processing blacklisted data IDs, an interceptor to prevent processing of blacklisted data, and corresponding unit tests to validate the new components. Changes
Sequence DiagramsequenceDiagram
participant Admin as Administrator
participant BlacklistResource as DataInfoIDBlacklistResource
participant MetaServer as Meta Server
participant SessionServer as Session Server
participant Interceptor as DataInfoIDBlacklistWrapperInterceptor
Admin->>BlacklistResource: Add/Delete DataInfoID
BlacklistResource->>MetaServer: Update Blacklist
MetaServer-->>SessionServer: Notify Blacklist Change
SessionServer->>SessionServer: Update Blacklist
alt Processing Data
Interceptor->>Interceptor: Check DataInfoID
alt DataInfoID is Blacklisted
Interceptor-->>: Skip Processing
else DataInfoID is Not Blacklisted
Interceptor->>: Continue Processing
end
end
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
hui-cha
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (12)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java (3)
35-35: Consider Adding Class-Level JavaDoc CommentThe class
DataInfoIDBlacklistResourcelacks a JavaDoc comment describing its purpose and usage. Adding a descriptive comment will improve maintainability and help other developers understand the class's role.+/** + * RESTful resource for managing DataInfoID blacklist operations such as adding and deleting Data IDs. + */ @Path("datainfoid/blacklist") public class DataInfoIDBlacklistResource {
75-79: Avoid Hard-Coding Error MessagesThe error message "Invalid data center" is hard-coded in multiple places. Hard-coding can lead to inconsistencies and makes localization difficult.
Define error messages as constants to improve maintainability:
+private static final String ERR_INVALID_DATA_CENTER = "Invalid data center"; ... if (!StringUtils.equals(dataCenter, clusterId)) { // Given dataCenter is not valid, return failure - return Result.failed("Invalid data center"); + return Result.failed(ERR_INVALID_DATA_CENTER); }
167-170: Enum Declaration Should Be Inside the ClassThe
Operationenum is declared outside the class. For better encapsulation and to avoid polluting the namespace, consider declaring it inside theDataInfoIDBlacklistResourceclass.-} - -enum Operation { + enum Operation { ADD, DELETE -} + } }server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java (3)
52-57: Initialize AtomicBoolean and WatchDog in DeclarationThe
startflag andwatchDogare initialized in the constructor, but they can be initialized at the point of declaration for clarity.-private AtomicBoolean start; +private AtomicBoolean start = new AtomicBoolean(false); -private WatchDog watchDog; +private WatchDog watchDog = new WatchDog();Then, remove the initialization from the constructor.
public FetchDataInfoIDBlackListService() { super(ValueConstants.SESSION_DATAID_BLACKLIST_DATA_ID, new DataInfoIDBlacklistStorage(INIT_VERSION, Collections.emptySet())); - this.start = new AtomicBoolean(false); - this.watchDog = new WatchDog(); }This makes the code more concise and ensures that the variables are always initialized.
74-94: Handle Exceptions More SpecificallyIn the
doProcessmethod, exceptions are caught broadly, which can obscure specific issues during processing.Consider catching specific exceptions like
JsonProcessingExceptionand logging more informative messages.try { // existing code } catch (JsonProcessingException e) { LOGGER.error("Failed to parse provideData: {}", provideData, e); return false; } catch (Exception e) { LOGGER.error("Unexpected exception during doProcess", e); return false; }This aids in debugging and maintenance.
96-108: Optimize Blacklist Check LogicThe method
isInBlackListretrieves the storage and dataInfoIds every time it's called. To improve performance, consider returning early if the storage or dataInfoIds are null or empty.public boolean isInBlackList(String dataInfoId) { DataInfoIDBlacklistStorage storage = this.storage.get(); return storage != null && storage.getDataInfoIds().contains(dataInfoId); }Since
dataInfoIdsis aSet, checking fornullorisEmptybeforecontainsis unnecessary becausecontainswill returnfalseif the set is empty.server/server/session/src/main/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptor.java (1)
43-46: Clarify Interceptor OrderingThe
getOrdermethod returns400, but it's unclear why this specific value is chosen. Clarify the reasoning or define it as a constant for better readability.+private static final int INTERCEPTOR_ORDER = 400; ... @Override public int getOrder() { - return 400; + return INTERCEPTOR_ORDER; }Consider documenting the choice of order.
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptorTest.java (3)
50-111: Organize Test Cases and Improve ReadabilityThe test method
test()combines multiple test cases, which can make it harder to identify which scenario fails if the test breaks.Consider splitting the test into separate methods for each scenario:
@Test public void testSubscriberNotAffectedByBlacklist() { ... } @Test public void testPublisherNotInBlacklistProcessesNormally() { ... } @Test public void testPublisherInBlacklistIsSkipped() { ... }This improves test maintainability and clarity.
114-128: Use Generics for Type Safety in MockIn
MockIsInBlackListAnswer, thedataInfoIdsset can benefit from explicit generic typing for better type safety.-private volatile Set dataInfoIds = new HashSet<>(); +private volatile Set<String> dataInfoIds = new HashSet<>();This ensures that only
Stringelements are added to the set.
130-164: Implement Assertions inTouchCheckerThe
TouchCheckerclass could include assertions to verify that thecall()method is invoked as expected.Add assertions or logging in the
call()method to enhance test robustness.@Override public Boolean call() { this.touched = true; Assert.assertNotNull("RegisterInvokeData should not be null", registerInvokeData); return true; }This helps catch unexpected scenarios during testing.
server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResourceTest.java (1)
106-133: Consider adding more notification test cases.While the current test verifies basic notification behavior, consider adding test cases for:
- Multiple consecutive notifications
- Empty blacklist notifications
- Error scenarios
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java (1)
59-180: Consider refactoring the large test method.The test method
testFetchDataAndCleanProvideris quite long and complex. Consider:
- Extracting publisher creation into a helper method
- Using @before for common setup
- Breaking into smaller, focused test cases
Example refactor for publisher creation:
private Publisher createPublisher(String dataId, String registerId, String sourcePort) { Publisher publisher = new Publisher(); publisher.setDataId(dataId); publisher.setInstanceId(this.instanceId); publisher.setGroup(this.group); publisher.setDataInfoId(String.format("%s#@#%s#@#%s", dataId, this.instanceId, this.group)); publisher.setPublishType(PublishType.NORMAL); publisher.setRegisterId(registerId); publisher.setSourceAddress(URL.valueOf("127.0.0.1:" + sourcePort)); publisher.setTargetAddress(URL.valueOf("127.0.0.1:9600")); return publisher; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java(1 hunks)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java(1 hunks)server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResourceTest.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptor.java(1 hunks)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java(1 hunks)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptorTest.java(1 hunks)
🔇 Additional comments (3)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java (1)
157-163: Improve Exception Handling in safeDeletePublisher
Exceptions caught in safeDeletePublisher are logged but not rethrown or handled further. If deletion fails, it might leave inconsistent state.
Verify if it's acceptable to silently continue after a deletion failure. Assess the impact on the system and consider implementing a retry mechanism or alerting.
✅ Verification successful
Based on the context gathered, I can now provide a final response:
Silent failure is acceptable for publisher deletion in blacklist cleanup
The safeDeletePublisher method is specifically used in the context of cleaning up blacklisted publishers through FetchDataInfoIDBlackListService. The silent failure is acceptable here because:
- It's part of a cleanup operation where the publisher is already being removed from the system due to blacklisting
- The deletion is attempted after the data has already been cleaned up at the Data layer through
writeDataAcceptor.accept() - The method name
safeDeletePublisherexplicitly indicates its intended behavior of handling failures gracefully - A failure to delete from the session store after successful data cleanup won't leave the system in an inconsistent state, as the publisher is already invalidated
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for failed publisher deletions in logs.
# Search the logs for "Delete publisher exception"
grep -r "Delete publisher exception" ./logs/
Length of output: 87
Script:
#!/bin/bash
# Let's analyze the error handling patterns in the codebase and the usage context of safeDeletePublisher
# Check how sessionDataStore.deleteById is implemented and its error handling
ast-grep --pattern 'deleteById($$$)'
# Find similar error handling patterns in the codebase
rg "catch \(Throwable" -A 2 -B 2
# Check where safeDeletePublisher is called from to understand the context
rg "safeDeletePublisher"
# Look for any retry mechanisms in the codebase
rg "retry|retryable|maxRetries" -i
Length of output: 69273
Script:
#!/bin/bash
# Let's analyze the context where safeDeletePublisher is used and how deletion failures are handled
# Find where safeDeletePublisher is called from
rg -B 5 -A 5 "safeDeletePublisher"
# Check the implementation of sessionDataStore.deleteById
ast-grep --pattern 'class $_ implements $_SessionDataStore {
$$$
deleteById($$$) {
$$$
}
$$$
}'
# Look for any retry mechanisms specifically around publisher deletion
rg -B 3 -A 3 "delete.*publisher.*retry" -i
Length of output: 3066
server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResourceTest.java (1)
58-104: LGTM! Comprehensive test coverage for blacklist operations.
The test thoroughly validates the addition and deletion of blacklist entries, including edge cases like deleting non-existent entries.
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java (1)
78-81: LGTM! Constant follows established naming and initialization patterns.
The new constant SESSION_DATAID_BLACKLIST_DATA_ID is well-placed and consistently formatted with other similar constants.
| private Result process(String dataCenter, String dataId, String group, String instanceId, Operation operation) { | ||
| // 1. 参数检查 | ||
| // 1.1. 检查 DataCenter 是否就是当前 Meta 的所属 DataCenter | ||
| String clusterId = defaultCommonConfig.getClusterId(TableEnum.PROVIDE_DATA.getTableName(), ValueConstants.SESSION_DATAID_BLACKLIST_DATA_ID); | ||
| if (!StringUtils.equals(dataCenter, clusterId)) { | ||
| // 给定的机房不是当前机房，那么拒绝添加黑名单，直接返回 | ||
| return Result.failed("Invalid data center"); | ||
| } | ||
|
|
||
| // 1.2. 检查要处理的 DataId 以及 Group 是否符合规则 | ||
| DataInfo dataInfo = new DataInfo(instanceId, dataId, group); | ||
| Tuple<Boolean, String> checkResult = this.checkDataInfoId(dataInfo); | ||
| if (!checkResult.o1) { | ||
| // 不符合规则，那么拒绝添加黑名单，直接返回 | ||
| return Result.failed("Invalid dataid: " + checkResult.o2); | ||
| } | ||
|
|
||
| // 2. 查询出当前黑名单列表 | ||
| DBResponse<PersistenceData> queryResponse = | ||
| this.provideDataService.queryProvideData(ValueConstants.SESSION_DATAID_BLACKLIST_DATA_ID); | ||
|
|
||
| // 3. 根据操作类型，添加 DataID 到列表中，或者删除列表中的 DataID，并保存 | ||
| Tuple<PersistenceData, Long> tuple = this.createNewPersistenceData(queryResponse, dataInfo, operation); | ||
| PersistenceData newPersistenceData = tuple.o1; | ||
| Long oldVersion = tuple.o2; | ||
| if (!this.provideDataService.saveProvideData(newPersistenceData, oldVersion)) { | ||
| // 保存失败 | ||
| return Result.failed("Save new black list fail"); | ||
| } | ||
|
|
||
| // 4. 保存成功则通知 Session 黑名单变化了 | ||
| ProvideDataChangeEvent provideDataChangeEvent = | ||
| new ProvideDataChangeEvent(ValueConstants.SESSION_DATAID_BLACKLIST_DATA_ID, | ||
| newPersistenceData.getVersion()); | ||
| this.provideDataNotifier.notifyProvideDataChange(provideDataChangeEvent); | ||
|
|
||
| return Result.success(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve Exception Handling and Logging
In the process method, exceptions are caught and logged, but specific exception types are not handled separately. This can make debugging difficult. Additionally, error messages returned to the client are generic.
Consider handling specific exceptions and providing more detailed error messages:
try {
// existing code
} catch (InvalidDataCenterException e) {
LOGGER.error("Invalid data center: {}", dataCenter, e);
return Result.failed("Invalid data center: " + dataCenter);
} catch (InvalidDataInfoIdException e) {
LOGGER.error("Invalid DataInfoId: {}", dataInfo.getDataInfoId(), e);
return Result.failed("Invalid DataInfoId: " + dataInfo.getDataInfoId());
} catch (Exception e) {
LOGGER.error("Unexpected exception in process method", e);
return Result.failed("Internal server error");
}Define custom exceptions like InvalidDataCenterException and InvalidDataInfoIdException for better clarity.
| DataInfo dataInfo = new DataInfo(instanceId, dataId, group); | ||
| Tuple<Boolean, String> checkResult = this.checkDataInfoId(dataInfo); | ||
| if (!checkResult.o1) { | ||
| // 不符合规则，那么拒绝添加黑名单，直接返回 | ||
| return Result.failed("Invalid dataid: " + checkResult.o2); | ||
| } |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| @POST | ||
| @Path("delete") | ||
| @Produces(MediaType.APPLICATION_JSON) | ||
| public Result deleteBlackList(String dataCenter, String dataId, String group, String instanceId) { | ||
| try { | ||
| return process(dataCenter, dataId, group, instanceId, Operation.DELETE); | ||
| } catch (Throwable throwable) { | ||
| LOGGER.error("Delete dataid black list exception", throwable); | ||
| return Result.failed("Delete dataid black list exception"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure Proper Input Validation and Handling
Similar to the addBlackList method, the deleteBlackList method should explicitly define how parameters are received and include input validation to prevent security issues.
Apply the same changes as suggested for the addBlackList method:
@POST
@Path("delete")
@Produces(MediaType.APPLICATION_JSON)
-public Result deleteBlackList(String dataCenter, String dataId, String group, String instanceId) {
+public Result deleteBlackList(
+ @FormParam("dataCenter") String dataCenter,
+ @FormParam("dataId") String dataId,
+ @FormParam("group") String group,
+ @FormParam("instanceId") String instanceId) {
try {
return process(dataCenter, dataId, group, instanceId, Operation.DELETE);
} catch (Throwable throwable) {Ensure input parameters are validated properly.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @POST | |
| @Path("delete") | |
| @Produces(MediaType.APPLICATION_JSON) | |
| public Result deleteBlackList(String dataCenter, String dataId, String group, String instanceId) { | |
| try { | |
| return process(dataCenter, dataId, group, instanceId, Operation.DELETE); | |
| } catch (Throwable throwable) { | |
| LOGGER.error("Delete dataid black list exception", throwable); | |
| return Result.failed("Delete dataid black list exception"); | |
| } | |
| } | |
| @POST | |
| @Path("delete") | |
| @Produces(MediaType.APPLICATION_JSON) | |
| public Result deleteBlackList( | |
| @FormParam("dataCenter") String dataCenter, | |
| @FormParam("dataId") String dataId, | |
| @FormParam("group") String group, | |
| @FormParam("instanceId") String instanceId) { | |
| try { | |
| return process(dataCenter, dataId, group, instanceId, Operation.DELETE); | |
| } catch (Throwable throwable) { | |
| LOGGER.error("Delete dataid black list exception", throwable); | |
| return Result.failed("Delete dataid black list exception"); | |
| } | |
| } |
| public Result addBlackList(String dataCenter, String dataId, String group, String instanceId) { | ||
| try { | ||
| return process(dataCenter, dataId, group, instanceId, Operation.ADD); | ||
| } catch (Throwable throwable) { | ||
| LOGGER.error("Save dataid black list exception", throwable); | ||
| return Result.failed("Save dataid black list exception"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Ensure Proper Input Validation and Handling
The method addBlackList accepts parameters directly without specifying the HTTP request handling of those parameters. It's important to explicitly define how these parameters are received, such as through query parameters or form data, and validate them to prevent potential security issues.
Consider specifying parameter annotations and adding input validation:
@POST
@Path("add")
@Produces(MediaType.APPLICATION_JSON)
-public Result addBlackList(String dataCenter, String dataId, String group, String instanceId) {
+public Result addBlackList(
+ @FormParam("dataCenter") String dataCenter,
+ @FormParam("dataId") String dataId,
+ @FormParam("group") String group,
+ @FormParam("instanceId") String instanceId) {
try {
return process(dataCenter, dataId, group, instanceId, Operation.ADD);
} catch (Throwable throwable) {Additionally, validate the input parameters to ensure they are not null or empty.
Committable suggestion skipped: line range outside the PR's diff.
| // 读取旧数据成功，其格式为 Json 字符串，解析出来 | ||
| PersistenceData oldPersistenceData = queryResponse.getEntity(); | ||
| String oldBlackListJson = oldPersistenceData.getData(); | ||
| Set<String> oldDataIdBlackList = JsonUtils.read(oldBlackListJson, new TypeReference<Set<String>>() {}); | ||
|
|
||
| // 添加或删除新的需要拉黑的数据 | ||
| if (Operation.ADD.equals(operation)) { | ||
| oldDataIdBlackList.add(dataInfo.getDataInfoId()); | ||
| } else { | ||
| oldDataIdBlackList.remove(dataInfo.getDataInfoId()); | ||
| } | ||
|
|
||
| // 创建新数据，并返回新数据以及旧数据的版本号 | ||
| PersistenceData newPersistenceData = PersistenceDataBuilder | ||
| .createPersistenceData(ValueConstants.SESSION_DATAID_BLACKLIST_DATA_ID, | ||
| JsonUtils.writeValueAsString(oldDataIdBlackList)); | ||
| return new Tuple<>(newPersistenceData, oldPersistenceData.getVersion()); | ||
| } else { | ||
| // 没有旧数据旧直接创建新的，旧数据的版本号设置为 0 | ||
| Set<String> dataIdBlackList = new HashSet<>(); | ||
| if (Operation.ADD.equals(operation)) { | ||
| dataIdBlackList.add(dataInfo.getDataInfoId()); | ||
| } | ||
| PersistenceData newPersistenceData = PersistenceDataBuilder | ||
| .createPersistenceData(ValueConstants.SESSION_DATAID_BLACKLIST_DATA_ID, | ||
| JsonUtils.writeValueAsString(dataIdBlackList)); | ||
| return new Tuple<>(newPersistenceData, 0L); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle Potential Null Pointer Exceptions
In the createNewPersistenceData method, when parsing oldBlackListJson, there is a risk of a NullPointerException if oldBlackListJson is null.
Add null checks before processing:
String oldBlackListJson = oldPersistenceData.getData();
+if (oldBlackListJson == null) {
+ oldBlackListJson = "[]";
+}
Set<String> oldDataIdBlackList = JsonUtils.read(oldBlackListJson, new TypeReference<Set<String>>() {});Alternatively, ensure that oldPersistenceData.getData() never returns null.
[error_handling]
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 读取旧数据成功，其格式为 Json 字符串，解析出来 | |
| PersistenceData oldPersistenceData = queryResponse.getEntity(); | |
| String oldBlackListJson = oldPersistenceData.getData(); | |
| Set<String> oldDataIdBlackList = JsonUtils.read(oldBlackListJson, new TypeReference<Set<String>>() {}); | |
| // 添加或删除新的需要拉黑的数据 | |
| if (Operation.ADD.equals(operation)) { | |
| oldDataIdBlackList.add(dataInfo.getDataInfoId()); | |
| } else { | |
| oldDataIdBlackList.remove(dataInfo.getDataInfoId()); | |
| } | |
| // 创建新数据，并返回新数据以及旧数据的版本号 | |
| PersistenceData newPersistenceData = PersistenceDataBuilder | |
| .createPersistenceData(ValueConstants.SESSION_DATAID_BLACKLIST_DATA_ID, | |
| JsonUtils.writeValueAsString(oldDataIdBlackList)); | |
| return new Tuple<>(newPersistenceData, oldPersistenceData.getVersion()); | |
| } else { | |
| // 没有旧数据旧直接创建新的，旧数据的版本号设置为 0 | |
| Set<String> dataIdBlackList = new HashSet<>(); | |
| if (Operation.ADD.equals(operation)) { | |
| dataIdBlackList.add(dataInfo.getDataInfoId()); | |
| } | |
| PersistenceData newPersistenceData = PersistenceDataBuilder | |
| .createPersistenceData(ValueConstants.SESSION_DATAID_BLACKLIST_DATA_ID, | |
| JsonUtils.writeValueAsString(dataIdBlackList)); | |
| return new Tuple<>(newPersistenceData, 0L); | |
| } | |
| } | |
| // 读取旧数据成功，其格式为 Json 字符串，解析出来 | |
| PersistenceData oldPersistenceData = queryResponse.getEntity(); | |
| String oldBlackListJson = oldPersistenceData.getData(); | |
| if (oldBlackListJson == null) { | |
| oldBlackListJson = "[]"; | |
| } | |
| Set<String> oldDataIdBlackList = JsonUtils.read(oldBlackListJson, new TypeReference<Set<String>>() {}); | |
| // 添加或删除新的需要拉黑的数据 | |
| if (Operation.ADD.equals(operation)) { | |
| oldDataIdBlackList.add(dataInfo.getDataInfoId()); | |
| } else { | |
| oldDataIdBlackList.remove(dataInfo.getDataInfoId()); | |
| } | |
| // 创建新数据，并返回新数据以及旧数据的版本号 | |
| PersistenceData newPersistenceData = PersistenceDataBuilder | |
| .createPersistenceData(ValueConstants.SESSION_DATAID_BLACKLIST_DATA_ID, | |
| JsonUtils.writeValueAsString(oldDataIdBlackList)); | |
| return new Tuple<>(newPersistenceData, oldPersistenceData.getVersion()); | |
| } else { | |
| // 没有旧数据旧直接创建新的，旧数据的版本号设置为 0 | |
| Set<String> dataIdBlackList = new HashSet<>(); | |
| if (Operation.ADD.equals(operation)) { | |
| dataIdBlackList.add(dataInfo.getDataInfoId()); | |
| } | |
| PersistenceData newPersistenceData = PersistenceDataBuilder | |
| .createPersistenceData(ValueConstants.SESSION_DATAID_BLACKLIST_DATA_ID, | |
| JsonUtils.writeValueAsString(dataIdBlackList)); | |
| return new Tuple<>(newPersistenceData, 0L); | |
| } | |
| } |
| public Boolean invokeCodeWrapper(WrapperInvocation<RegisterInvokeData, Boolean> invocation) throws Exception { | ||
| RegisterInvokeData registerInvokeData = invocation.getParameterSupplier().get(); | ||
| BaseInfo storeData = (BaseInfo) registerInvokeData.getStoreData(); | ||
|
|
||
| // 这里我们只拦截 Publisher | ||
| if (DataType.PUBLISHER == storeData.getDataType()) { | ||
| Publisher publisher = (Publisher) storeData; | ||
| String dataInfoId = publisher.getDataInfoId(); | ||
| if (this.fetchDataInfoIDBlackListService.isInBlackList(dataInfoId)) { | ||
| // 命中规则，跳过 Pub | ||
| return true; | ||
| } else { | ||
| // 没命中则继续处理 | ||
| return invocation.proceed(); | ||
| } | ||
| } | ||
|
|
||
| // 非 Publisher 也继续处理 | ||
| return invocation.proceed(); | ||
| } |
There was a problem hiding this comment.
Handle Null Pointer Exception Risk
In invokeCodeWrapper, the storeData may be null, leading to a NullPointerException when accessing getDataType().
Add null checks before proceeding.
if (storeData == null) {
return invocation.proceed();
}Place this check before accessing storeData.getDataType().
| @Autowired | ||
| private FetchDataInfoIDBlackListService fetchDataInfoIDBlackListService; | ||
|
|
There was a problem hiding this comment.
Autowired Field Should Be Final or Have Setter
The fetchDataInfoIDBlackListService field is annotated with @Autowired but not marked as final or provided with a setter for Spring to inject properly.
Ensure proper dependency injection by adding a setter or marking the field as final and using constructor injection.
-@Autowired
-private FetchDataInfoIDBlackListService fetchDataInfoIDBlackListService;
+private final FetchDataInfoIDBlackListService fetchDataInfoIDBlackListService;
+
+@Autowired
+public DataInfoIDBlacklistWrapperInterceptor(FetchDataInfoIDBlackListService fetchDataInfoIDBlackListService) {
+ this.fetchDataInfoIDBlackListService = fetchDataInfoIDBlackListService;
+}This ensures that the dependency is properly injected.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @Autowired | |
| private FetchDataInfoIDBlackListService fetchDataInfoIDBlackListService; | |
| private final FetchDataInfoIDBlackListService fetchDataInfoIDBlackListService; | |
| @Autowired | |
| public DataInfoIDBlacklistWrapperInterceptor(FetchDataInfoIDBlackListService fetchDataInfoIDBlackListService) { | |
| this.fetchDataInfoIDBlackListService = fetchDataInfoIDBlackListService; | |
| } |
| @Test | ||
| public void testCleanPublisher() { | ||
|
|
||
| } |
There was a problem hiding this comment.
Remove empty test method.
The empty test method testCleanPublisher() should either be implemented or removed.
| fetchDataInfoIDBlackListService.start(); | ||
|
|
||
| // 等 300ms，让它处理完 | ||
| this.sleep(300L, TimeUnit.MILLISECONDS); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace Thread.sleep with more reliable test synchronization.
Using Thread.sleep for test synchronization is fragile and could lead to flaky tests. Consider using CountDownLatch or Awaitility for more reliable test synchronization.
Example using CountDownLatch:
private final CountDownLatch latch = new CountDownLatch(1);
// In the service, call latch.countDown() after processing
// In test:
Assert.assertTrue("Processing timed out", latch.await(300, TimeUnit.MILLISECONDS));Also applies to: 163-164
| private void sleep(Long time, TimeUnit timeUnit) { | ||
| try { | ||
| Thread.sleep(timeUnit.toMillis(time)); | ||
| } catch (InterruptedException interruptedException) { | ||
| // 忽略 | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve interrupted exception handling.
The current implementation silently ignores InterruptedException. Consider either:
- Restoring the interrupt flag:
Thread.currentThread().interrupt(); - Or using a more robust waiting mechanism like CountDownLatch
private void sleep(Long time, TimeUnit timeUnit) {
try {
Thread.sleep(timeUnit.toMillis(time));
} catch (InterruptedException interruptedException) {
- // 忽略
+ Thread.currentThread().interrupt();
}
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private void sleep(Long time, TimeUnit timeUnit) { | |
| try { | |
| Thread.sleep(timeUnit.toMillis(time)); | |
| } catch (InterruptedException interruptedException) { | |
| // 忽略 | |
| } | |
| } | |
| private void sleep(Long time, TimeUnit timeUnit) { | |
| try { | |
| Thread.sleep(timeUnit.toMillis(time)); | |
| } catch (InterruptedException interruptedException) { | |
| Thread.currentThread().interrupt(); | |
| } | |
| } |
hui-cha
force-pushed
the
feature/dataid_blacklist
branch
from
6ca5ae6 to
7726470
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java (1)
72-93: Inconsistent code comment languageSeveral comments in the
doProcessandsafeCleanPublishermethods are written in Chinese, while the rest of the codebase appears to use English. For consistency and to aid understanding among all team members, it's recommended to use English for all code comments.Also applies to: 110-142
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java (1)
Line range hint
799-826: Ensure consistent naming for bean methodsThe method
wrapperInterceptor()should be renamed todataInfoIDBlacklistWrapperInterceptor()to maintain consistency with other bean method names. This enhances readability and aligns with the naming convention used throughout the class.Apply this diff to rename the method and update its usage:
public WrapperInterceptorManager wrapperInterceptorManager() { WrapperInterceptorManager mgr = new WrapperInterceptorManager(); mgr.addInterceptor(clientCheckWrapperInterceptor()); mgr.addInterceptor(blacklistWrapperInterceptor()); mgr.addInterceptor(accessLimitWrapperInterceptor()); mgr.addInterceptor(clientOffWrapperInterceptor()); - mgr.addInterceptor(wrapperInterceptor()); + mgr.addInterceptor(dataInfoIDBlacklistWrapperInterceptor()); return mgr; } -@Bean -public WrapperInterceptor wrapperInterceptor() { +@Bean +public WrapperInterceptor dataInfoIDBlacklistWrapperInterceptor() { return new DataInfoIDBlacklistWrapperInterceptor(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java(1 hunks)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java(1 hunks)server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResourceTest.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java(5 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptor.java(1 hunks)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java(1 hunks)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptorTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptor.java
🚧 Files skipped from review as they are similar to previous changes (5)
- server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java
- server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResourceTest.java
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptorTest.java
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java
🔇 Additional comments (2)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java (1)
110-142: Potential Concurrent Modification Exception
In the safeCleanPublisher() method, the code iterates over publishers retrieved from sessionDataStore. If sessionDataStore is modified concurrently, this may lead to a ConcurrentModificationException.
Ensure thread safety by synchronizing access to sessionDataStore or by using thread-safe collections and methods.
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java (1)
948-955: Verify the correct processor registration method is used
In the fetchDataInfoIDBlackListService bean, you're adding the processor using addSystemDataProcessor(). Similar services sometimes use addSystemDataPersistenceProcessor(). Please verify if addSystemDataPersistenceProcessor() should be used here instead to ensure the processor is registered correctly.
Run the following script to check the usage of processor registration methods:
This script will help determine if fetchDataInfoIDBlackListService should use addSystemDataProcessor() or addSystemDataPersistenceProcessor() based on how similar services are registered.
✅ Verification successful
The registration method addSystemDataProcessor() is correctly used
Based on the search results, the registration method choice appears correct. The codebase shows a clear pattern:
addSystemDataProcessor()is used for services that handle system configurations and runtime features (black lists, gray push switches, compression settings, etc.)addSystemDataPersistenceProcessor()is used for services that manage system state and lifecycle (shutdown, circuit breaker, client offline addresses)
Since FetchDataInfoIDBlackListService is a configuration-related service similar to FetchBlackListService, the current registration using addSystemDataProcessor() is appropriate.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify usage of processor registration methods in SessionProvideDataConfiguration
# Search for usage of addSystemDataProcessor
rg --multiline-dotall 'systemPropertyProcessorManager\.addSystemDataProcessor\(' -A2 -B5 server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java
# Search for usage of addSystemDataPersistenceProcessor
rg --multiline-dotall 'systemPropertyProcessorManager\.addSystemDataPersistenceProcessor\(' -A2 -B5 server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java
Length of output: 4246
hui-cha
force-pushed
the
feature/dataid_blacklist
branch
from
7726470 to
74bd786
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java (2)
155-157:⚠️ Potential issueImplement Proper DataInfoId Validation Logic
The
checkDataInfoId()method currently returnstruewithout performing any validation. This could allow invalidDataInfoIds to be added to the blacklist.Consider implementing validation logic to ensure the
DataInfoIdconforms to expected patterns or business rules. For example:protected Tuple<Boolean, String> checkDataInfoId(DataInfo dataInfo) { + if (dataInfo == null || StringUtils.isEmpty(dataInfo.getDataInfoId())) { + return new Tuple<>(false, "DataInfoId is null or empty"); + } + // Add additional validation as needed return new Tuple<>(true, ""); }
126-129:⚠️ Potential issueHandle Potential NullPointerException in
createNewPersistenceData()In the
createNewPersistenceData()method,oldPersistenceData.getData()might returnnull, leading to aNullPointerExceptionwhen parsingoldBlackListJson.[error_handling]
Add a null check before processing:
String oldBlackListJson = oldPersistenceData.getData(); + if (oldBlackListJson == null) { + oldBlackListJson = "[]"; + } Set<String> oldDataIdBlackList = JsonUtils.read(oldBlackListJson, new TypeReference<Set<String>>() {});
🧹 Nitpick comments (1)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java (1)
Line range hint
1-955: Consider adding configuration documentationThe addition of the DataID blacklist functionality introduces significant new configuration options. Consider adding documentation in the class-level JavaDoc to:
- Explain the blacklist mechanism
- Document the configuration key
session.datainfoid.blacklist#@#9600#@#CONFIG- Describe the interaction between Meta and Session components for blacklist synchronization
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java(1 hunks)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java(3 hunks)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java(1 hunks)server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResourceTest.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java(5 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptor.java(1 hunks)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java(1 hunks)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptor.java
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptorTest.java
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java
- server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResourceTest.java
🔇 Additional comments (6)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java (4)
48-145: LGTM: Import organization aligns with the PR objectives
The new imports support the DataID blacklist functionality by including necessary components for:
- Client/Data node services
- Provide data services for blacklist management
- Push-related services for notification
- Wrapper interceptors for request filtering
823-826: LGTM: Clean bean definition for the new interceptor
The DataInfoIDBlacklistWrapperInterceptor is properly configured as a Spring bean.
798-800: Verify interceptor ordering in WrapperInterceptorManager
The DataInfoIDBlacklistWrapperInterceptor is added last in the chain. Consider if this ordering is optimal - it should run after client checks but before access limits.
✅ Verification successful
The interceptor ordering is correct and optimal
The interceptors are ordered by their getOrder() values in ascending order, as implemented in the Comp comparator. The current order is:
- AccessLimitWrapperInterceptor (order=0)
- ClientCheckWrapperInterceptor (order=100)
- BlacklistWrapperInterceptor (order=200)
- ClientOffWrapperInterceptor (order=300)
- DataInfoIDBlacklistWrapperInterceptor (order=400)
This ordering is optimal because:
- Access limits are checked first as a basic rate-limiting mechanism
- Client validation happens next to ensure the client is valid
- General blacklist checks follow
- Client offline status is verified before specific data checks
- DataInfoID blacklist is checked last, which is correct since it's a specific data-level check after all client-level validations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of WrapperInterceptorManager to understand interceptor execution order
# Search for the WrapperInterceptorManager implementation
ast-grep --pattern 'class WrapperInterceptorManager {
$$$
addInterceptor($_) {
$$$
}
$$$
}'
Length of output: 105
Script:
#!/bin/bash
# Let's try a different approach to find the WrapperInterceptorManager implementation
# First, let's find the file containing this class
rg -l "class WrapperInterceptorManager"
# Then let's look for how interceptors are stored and executed
rg -A 10 "class WrapperInterceptorManager"
# Also search for any documentation or comments about interceptor ordering
rg -B 2 -A 5 "WrapperInterceptorManager"
# Let's also check what types of interceptors are being added
rg "addInterceptor\(" -A 1
Length of output: 14038
Script:
#!/bin/bash
# Let's check the Comp class implementation to understand the ordering logic
rg -A 10 "class Comp"
# Also check if WrapperInterceptor has any ordering-related methods or interfaces
rg -A 5 "interface WrapperInterceptor"
# Let's also look for any order-related methods in the interceptor implementations
rg "getOrder|@Order" -A 2
Length of output: 41029
949-955: Verify system property processor registration
The FetchDataInfoIDBlackListService is correctly registered as a system data processor. However, verify if it should be a persistence processor instead, similar to other blacklist-related services.
✅ Verification successful
The registration of FetchDataInfoIDBlackListService is correct as a system data processor
Based on the code analysis:
-
FetchDataInfoIDBlackListService is correctly registered as a system data processor because:
- It manages a blacklist of dataInfoIDs that need to be monitored and cleaned up periodically
- It doesn't require persistence as it's an in-memory filtering mechanism
- It extends AbstractFetchSystemPropertyService, not AbstractFetchPersistenceSystemProperty
-
In contrast, FetchBlackListService is also correctly registered as a system data processor because:
- It handles IP-based blacklisting for client connections
- It operates on runtime connection management
- Like FetchDataInfoIDBlackListService, it extends AbstractFetchSystemPropertyService
Both services are correctly configured for their respective purposes - runtime filtering and monitoring rather than data persistence.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how other blacklist services are registered
# Search for similar blacklist service registrations
rg -A 2 "systemPropertyProcessorManager.add.*Processor.*Blacklist"
Length of output: 68
Script:
#!/bin/bash
# Let's try a broader search for blacklist services and their registration patterns
# Search for blacklist service class definitions and their processor type
rg -l "class.*BlackList.*Service"
# Search for any system processor registration patterns
rg -A 2 "systemPropertyProcessorManager.add"
# Search for persistence processor patterns
rg -A 2 "PersistenceProcessor|addPersistenceProcessor"
Length of output: 12044
Script:
#!/bin/bash
# Let's examine the implementation of both blacklist services to understand their nature
# Check FetchBlackListService implementation
ast-grep --pattern 'class FetchBlackListService {
$$$
}'
# Check FetchDataInfoIDBlackListService implementation
ast-grep --pattern 'class FetchDataInfoIDBlackListService {
$$$
}'
Length of output: 131
Script:
#!/bin/bash
# Let's try to read the actual content of both blacklist service files
# Read FetchBlackListService
cat server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchBlackListService.java
# Read FetchDataInfoIDBlackListService
cat server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java
Length of output: 15834
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java (1)
110-142: Potential Concurrent Modification Exception in safeCleanPublisher()
In the safeCleanPublisher() method, modifying sessionDataStore while iterating over its elements may lead to a ConcurrentModificationException. Ensure thread safety by synchronizing access or using concurrent collections.
[concurrent_programming_issue]
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java (1)
399-403: Integration of DataInfoIDBlacklistResource Looks Good
The addition of the dataInfoIDBlacklistResource() method correctly integrates the new resource into the MetaServer configuration.
| } catch (Throwable throwable) { | ||
| LOGGER.error("Process new dataInfoId black list exception", throwable); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Avoid Catching Throwable in Exception Handling
Catching Throwable may mask serious errors like OutOfMemoryError. It's safer to catch Exception instead.
Apply this diff to catch Exception instead of Throwable:
- } catch (Throwable throwable) {
- LOGGER.error("Process new dataInfoId black list exception", throwable);
- return false;
- }
+ } catch (Exception e) {
+ LOGGER.error("Process new dataInfoId black list exception", e);
+ return false;
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (Throwable throwable) { | |
| LOGGER.error("Process new dataInfoId black list exception", throwable); | |
| return false; | |
| } | |
| } catch (Exception e) { | |
| LOGGER.error("Process new dataInfoId black list exception", e); | |
| return false; | |
| } |
| } catch (Throwable throwable) { | ||
| LOGGER.error("Clean data exception", throwable); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid Catching Throwable in safeCleanData()
Catching Throwable may mask critical errors. It's better to catch Exception instead.
Apply this diff:
- } catch (Throwable throwable) {
- LOGGER.error("Clean data exception", throwable);
- }
+ } catch (Exception e) {
+ LOGGER.error("Clean data exception", e);
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (Throwable throwable) { | |
| LOGGER.error("Clean data exception", throwable); | |
| } | |
| } | |
| } catch (Exception e) { | |
| LOGGER.error("Clean data exception", e); | |
| } | |
| } |
| LOGGER.error("[WatchDog] clean publisher exception", throwable); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid Catching Throwable in safeCleanPublisher()
Catching Throwable can suppress critical errors. Catch Exception instead to prevent masking serious issues.
Apply this diff:
- } catch (Throwable throwable) {
- LOGGER.error("[WatchDog] clean publisher exception", throwable);
- }
+ } catch (Exception e) {
+ LOGGER.error("[WatchDog] clean publisher exception", e);
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LOGGER.error("[WatchDog] clean publisher exception", throwable); | |
| } | |
| } | |
| LOGGER.error("[WatchDog] clean publisher exception", e); | |
| } | |
| } |
| } catch (Throwable throwable) { | ||
| LOGGER.error("Delete publisher exception", throwable); | ||
| } | ||
| } |
There was a problem hiding this comment.
Avoid Catching Throwable in safeDeletePublisher()
Catching Throwable can suppress serious errors. Replace with catching Exception to avoid masking critical issues.
Apply this diff:
- } catch (Throwable throwable) {
- LOGGER.error("Delete publisher exception", throwable);
- }
+ } catch (Exception e) {
+ LOGGER.error("Delete publisher exception", e);
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (Throwable throwable) { | |
| LOGGER.error("Delete publisher exception", throwable); | |
| } | |
| } | |
| } catch (Exception e) { | |
| LOGGER.error("Delete publisher exception", e); | |
| } | |
| } |
| } catch (Throwable throwable) { | ||
| LOGGER.error("Save dataid black list exception", throwable); | ||
| return Result.failed("Save dataid black list exception"); | ||
| } |
There was a problem hiding this comment.
Avoid Catching Throwable in addBlackList()
Catching Throwable may suppress critical errors like OutOfMemoryError. It's safer to catch Exception instead.
Apply this diff:
- } catch (Throwable throwable) {
- LOGGER.error("Save dataid black list exception", throwable);
- return Result.failed("Save dataid black list exception");
- }
+ } catch (Exception e) {
+ LOGGER.error("Save dataid black list exception", e);
+ return Result.failed("Save dataid black list exception");
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (Throwable throwable) { | |
| LOGGER.error("Save dataid black list exception", throwable); | |
| return Result.failed("Save dataid black list exception"); | |
| } | |
| } catch (Exception e) { | |
| LOGGER.error("Save dataid black list exception", e); | |
| return Result.failed("Save dataid black list exception"); | |
| } |
| } catch (Throwable throwable) { | ||
| LOGGER.error("Delete dataid black list exception", throwable); | ||
| return Result.failed("Delete dataid black list exception"); | ||
| } |
There was a problem hiding this comment.
Avoid Catching Throwable in deleteBlackList()
Similar to addBlackList(), catching Throwable can mask critical errors. Catch Exception instead.
Apply this diff:
- } catch (Throwable throwable) {
- LOGGER.error("Delete dataid black list exception", throwable);
- return Result.failed("Delete dataid black list exception");
- }
+ } catch (Exception e) {
+ LOGGER.error("Delete dataid black list exception", e);
+ return Result.failed("Delete dataid black list exception");
+ }📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (Throwable throwable) { | |
| LOGGER.error("Delete dataid black list exception", throwable); | |
| return Result.failed("Delete dataid black list exception"); | |
| } | |
| } catch (Exception e) { | |
| LOGGER.error("Delete dataid black list exception", e); | |
| return Result.failed("Delete dataid black list exception"); | |
| } |
hui-cha
force-pushed
the
feature/dataid_blacklist
branch
from
74bd786 to
7564b80
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java (1)
83-120: Improve exception handling and logging inprocessmethodIn the
processmethod, exceptions are not handled specifically, which could make debugging difficult. Also, the error messages returned to the client are generic.Consider handling specific exceptions and providing detailed error messages:
private Result process(String dataCenter, String dataId, String group, String instanceId, Operation operation) { try { // Existing logic } catch (InvalidDataCenterException e) { LOGGER.error("Invalid data center: {}", dataCenter, e); return Result.failed("Invalid data center: " + dataCenter); } catch (InvalidDataInfoIdException e) { LOGGER.error("Invalid DataInfoId: {}", dataInfo.getDataInfoId(), e); return Result.failed("Invalid DataInfoId: " + dataInfo.getDataInfoId()); } catch (Exception e) { LOGGER.error("Unexpected exception in process method", e); return Result.failed("Internal server error"); } }Define custom exceptions like
InvalidDataCenterExceptionandInvalidDataInfoIdExceptionfor better clarity.server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java (1)
799-799: Rename the methodwrapperInterceptor()todataInfoIDBlacklistWrapperInterceptor()for clarity and consistencyThe method
wrapperInterceptor()has a generic name that doesn't clearly indicate its purpose. For better clarity and to maintain consistency with other interceptor bean methods likeclientCheckWrapperInterceptor()andblacklistWrapperInterceptor(), consider renaming it todataInfoIDBlacklistWrapperInterceptor().Apply this diff to rename the method and update its usage:
@@ -799,1 +799,1 @@ - mgr.addInterceptor(wrapperInterceptor()); + mgr.addInterceptor(dataInfoIDBlacklistWrapperInterceptor());@@ -824,3 +824,3 @@ - public WrapperInterceptor wrapperInterceptor() { + public WrapperInterceptor dataInfoIDBlacklistWrapperInterceptor() { return new DataInfoIDBlacklistWrapperInterceptor(); }Also applies to: 823-826
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java(1 hunks)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java(3 hunks)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java(1 hunks)server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResourceTest.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfig.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfigBean.java(3 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java(5 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptor.java(1 hunks)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java(1 hunks)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java
- server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptorTest.java
- server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResourceTest.java
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptor.java
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java
🔇 Additional comments (7)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java (4)
125-129: Handle potential NullPointerException when parsing oldBlackListJson
If oldBlackListJson is null, calling JsonUtils.read on it could throw a NullPointerException.
[error_handling]
Add a null check before parsing:
String oldBlackListJson = oldPersistenceData.getData();
+ if (oldBlackListJson == null) {
+ oldBlackListJson = "[]";
+ }
Set<String> oldDataIdBlackList = JsonUtils.read(oldBlackListJson, new TypeReference<Set<String>>() {});77-80: 🛠️ Refactor suggestion
Avoid catching Throwable in exception handling
Similarly, in the deleteBlackList method, catching Throwable is not recommended. You should catch Exception to handle expected exceptions without suppressing serious errors.
Apply this diff:
- } catch (Throwable throwable) {
- LOGGER.error("Delete dataid black list exception", throwable);
- return Result.failed("Delete dataid black list exception");
- }
+ } catch (Exception e) {
+ LOGGER.error("Delete dataid black list exception", e);
+ return Result.failed("Delete dataid black list exception");
+ }Likely invalid or redundant comment.
62-65: 🛠️ Refactor suggestion
Avoid catching Throwable in exception handling
Catching Throwable can unintentionally catch errors that are not meant to be handled, such as OutOfMemoryError or StackOverflowError. It's safer to catch Exception instead to avoid suppressing critical errors.
Apply this diff to catch Exception instead:
- } catch (Throwable throwable) {
- LOGGER.error("Save dataid black list exception", throwable);
- return Result.failed("Save dataid black list exception");
- }
+ } catch (Exception e) {
+ LOGGER.error("Save dataid black list exception", e);
+ return Result.failed("Save dataid black list exception");
+ }Likely invalid or redundant comment.
93-98:
Implement validation logic in checkDataInfoId method
The checkDataInfoId method currently always returns true without performing any actual validation. This could lead to invalid DataInfoIds being processed.
Implement proper validation logic. For example:
protected Tuple<Boolean, String> checkDataInfoId(DataInfo dataInfo) {
+ if (dataInfo == null || StringUtils.isEmpty(dataInfo.getDataInfoId())) {
+ return new Tuple<>(false, "DataInfoId is null or empty");
+ }
+ // Additional validation rules can be added here
return new Tuple<>(true, "");
}Likely invalid or redundant comment.
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfig.java (1)
212-213: LGTM!
The addition of the new method int getScanPublisherInDataInfoIdBlackListIntervalMillis(); properly extends the interface to support the new configuration.
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfigBean.java (1)
1461-1468: Ensure consistency in getter and setter methods
The newly added getter and setter for scanPublisherInDataInfoIdBlackListIntervalMillis are correctly implemented. Ensure that this property is properly initialized and used wherever necessary.
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java (1)
952-955: Ensure FetchDataInfoIDBlackListService is properly registered and utilized
The FetchDataInfoIDBlackListService is instantiated and added to the SystemPropertyProcessorManager, but it's not evident how it's accessed by other components, such as the DataInfoIDBlacklistWrapperInterceptor. Verify that all components that rely on this service have the necessary dependencies injected.
Run the following script to search for usages of FetchDataInfoIDBlackListService and ensure it is properly injected where required:
✅ Verification successful
FetchDataInfoIDBlackListService is properly integrated and utilized
The service is correctly wired into the system:
- It's properly autowired in
DataInfoIDBlacklistWrapperInterceptorusing@Autowired - The service is registered with
SystemPropertyProcessorManagerin the Spring configuration - The interceptor has both constructor injection and setter injection support
- Unit tests demonstrate proper usage and integration
- The service extends
AbstractFetchSystemPropertyServiceand is properly configured with required dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of FetchDataInfoIDBlackListService and check for proper dependency injection.
# Search for constructor injections of FetchDataInfoIDBlackListService
rg 'new\s+\w+\(.*FetchDataInfoIDBlackListService.*\)' -t java
# Search for methods that accept FetchDataInfoIDBlackListService as a parameter
rg 'public\s+\w+\s+\w+\(.*FetchDataInfoIDBlackListService.*\)' -t java
# Search for fields of type FetchDataInfoIDBlackListService with @Autowired annotation
rg '@Autowired\s+.*FetchDataInfoIDBlackListService' -t java
Length of output: 474
Script:
#!/bin/bash
# Let's check how DataInfoIDBlacklistWrapperInterceptor uses the service and if it's properly configured
# Check the implementation of DataInfoIDBlacklistWrapperInterceptor
ast-grep --pattern 'class DataInfoIDBlacklistWrapperInterceptor {
$$$
}'
# Check for any Spring configuration that wires these components together
rg -A 5 'DataInfoIDBlacklistWrapperInterceptor' -t java
# Check if there are any other usages of this service
rg 'FetchDataInfoIDBlackListService' -t java
Length of output: 12317
| public Result addBlackList(@FormParam("dataCenter") String dataCenter, | ||
| @FormParam("dataId") String dataId, | ||
| @FormParam("group") String group, | ||
| @FormParam("instanceId") String instanceId) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify parameter annotations for method inputs
Currently, the parameters in the addBlackList method are annotated with @FormParam, but it's important to ensure that all parameters are correctly annotated and validated to prevent injection attacks or misuse.
Ensure all parameters have the appropriate annotations and add input validation:
@POST
@Path("add")
@Produces(MediaType.APPLICATION_JSON)
public Result addBlackList(
@FormParam("dataCenter") String dataCenter,
@FormParam("dataId") String dataId,
@FormParam("group") String group,
@FormParam("instanceId") String instanceId) {
// Input validation can be added here
try {
return process(dataCenter, dataId, group, instanceId, Operation.ADD);
} catch (Exception e) {
LOGGER.error("Save dataid black list exception", e);
return Result.failed("Save dataid black list exception");
}
}| public Result deleteBlackList(@FormParam("dataCenter") String dataCenter, | ||
| @FormParam("dataId") String dataId, | ||
| @FormParam("group") String group, | ||
| @FormParam("instanceId") String instanceId) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Specify parameter annotations for method inputs
Similarly, in the deleteBlackList method, ensure that all parameters are properly annotated and validated.
Apply this change:
@POST
@Path("delete")
@Produces(MediaType.APPLICATION_JSON)
public Result deleteBlackList(
@FormParam("dataCenter") String dataCenter,
@FormParam("dataId") String dataId,
@FormParam("group") String group,
@FormParam("instanceId") String instanceId) {
try {
return process(dataCenter, dataId, group, instanceId, Operation.DELETE);
} catch (Exception e) {
LOGGER.error("Delete dataid black list exception", e);
return Result.failed("Delete dataid black list exception");
}
}| @Bean | ||
| public WrapperInterceptor wrapperInterceptor() { | ||
| return new DataInfoIDBlacklistWrapperInterceptor(); | ||
| } |
There was a problem hiding this comment.
Inject FetchDataInfoIDBlackListService into DataInfoIDBlacklistWrapperInterceptor
The DataInfoIDBlacklistWrapperInterceptor likely depends on FetchDataInfoIDBlackListService to access the blacklist data. Currently, it's instantiated without injecting this dependency, which may lead to a NullPointerException or unexpected behavior at runtime. Ensure that the interceptor is properly initialized with the required service.
Apply this diff to inject the required dependency:
@@ -824,3 +824,3 @@
- public WrapperInterceptor dataInfoIDBlacklistWrapperInterceptor() {
- return new DataInfoIDBlacklistWrapperInterceptor();
+ public WrapperInterceptor dataInfoIDBlacklistWrapperInterceptor(FetchDataInfoIDBlackListService fetchDataInfoIDBlackListService) {
+ return new DataInfoIDBlacklistWrapperInterceptor(fetchDataInfoIDBlackListService);
}Update the wrapperInterceptorManager() method to use the renamed method and let Spring inject the dependency:
@@ -799,1 +799,1 @@
- mgr.addInterceptor(wrapperInterceptor());
+ mgr.addInterceptor(dataInfoIDBlacklistWrapperInterceptor());Also applies to: 952-955
…quests from clients
hui-cha
force-pushed
the
feature/dataid_blacklist
branch
from
7564b80 to
0d7342c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/sessionserver/QueryPublisherRequest.java (2)
13-13: Consider whetherserialVersionUIDis needed.
Since this class is newly created, providing aserialVersionUIDensures compatibility if this class is ever serialized. This is acceptable, but confirm whether this class will actually require serialization across different application versions.
27-28: Typographical issue in the format string.
There appears to be an extra brace in the format: "QueryPublisherRequest={}}". This could lead to confusion in logs.To fix, you could change line 27 to:
-return StringFormatter.format("QueryPublisherRequest={}}", dataInfoId); +return StringFormatter.format("QueryPublisherRequest={}", dataInfoId);server/common/model/src/main/java/com/alipay/sofa/registry/common/model/sessionserver/SimplePublisher.java (1)
41-43: Check placeholder alignment in toString.
The placeholders "app={},clientId={},add={}" do not precisely reflect the field names. “add” might be better named “address” to reduce confusion in logs.-return StringFormatter.format( - "SimplePublisher{app={},clientId={},add={}}", appName, clientId, sourceAddress); +return StringFormatter.format( + "SimplePublisher{appName={}, clientId={}, sourceAddress={}}", + appName, clientId, sourceAddress);server/server/session/src/main/java/com/alipay/sofa/registry/server/session/remoting/console/handler/QueryPublisherRequestHandler.java (1)
21-23: Promote constructor injection over field injection.
Using field injection with@Autowiredsometimes complicates testing and reduces clarity. Consider replacing field injection with constructor injection.server/common/model/src/main/java/com/alipay/sofa/registry/common/model/PublisherUtils.java (2)
20-20: Use consistent collection libraries.You're using Guava's Lists and the Apache Commons CollectionUtils, which is perfectly fine. However, ensure you consistently use either Guava or Apache Commons for collection utilities to maintain clarity throughout the codebase.
111-120: Efficient handling of empty collections.While the logic is correct, consider adding early returns for null checks or explicitly stating the expected behavior if the publishers parameter is null. This can prevent potential issues or confusion for future maintainers.
+if (publishers == null) { + return Collections.emptyList(); +} if (CollectionUtils.isEmpty(publishers)) { return Collections.emptyList(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/PublisherUtils.java(2 hunks)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java(1 hunks)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/sessionserver/QueryPublisherRequest.java(1 hunks)server/common/model/src/main/java/com/alipay/sofa/registry/common/model/sessionserver/SimplePublisher.java(1 hunks)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java(3 hunks)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java(1 hunks)server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResourceTest.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfig.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfigBean.java(3 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java(7 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/remoting/console/handler/QueryPublisherRequestHandler.java(1 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/SessionDigestResource.java(4 hunks)server/server/session/src/main/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptor.java(1 hunks)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java(1 hunks)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/remoting/console/handler/QueryPublisherRequestHandlerTest.java(1 hunks)server/server/session/src/test/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptorTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- server/common/model/src/main/java/com/alipay/sofa/registry/common/model/constants/ValueConstants.java
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfig.java
- server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResourceTest.java
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptorTest.java
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfigBean.java
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/wrapper/DataInfoIDBlacklistWrapperInterceptor.java
- server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListServiceTest.java
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java
👮 Files not reviewed due to content moderation or server errors (2)
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchDataInfoIDBlackListService.java
- server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java
🔇 Additional comments (7)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/sessionserver/SimplePublisher.java (1)
15-19: Prefer constructor-based injection for immutability
All fields are final and assigned in the constructor, which is a good approach to ensure immutability. This is aligned with best practices.
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/remoting/console/handler/QueryPublisherRequestHandler.java (2)
47-50: ExecutorManager usage check.
While the setter for ExecutorManager exists, the manager is not referenced within this class. Confirm it will be utilized in subsequent code or remove the setter to reduce clutter.
✅ Verification successful
Let me verify if the ExecutorManager is being used within the QueryPublisherRequestHandler class.
Let me check if this class inherits any functionality from its parent class that might use the executorManager.
The executorManager setter is justified - field is used via parent class
The executorManager field is actually used through inheritance. The QueryPublisherRequestHandler extends AbstractConsoleHandler which uses the executorManager to provide thread pool execution via getExecutor() method that returns executorManager.getConsoleExecutor(). The setter is marked with @VisibleForTesting and is used for test injection.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for usage of executorManager within QueryPublisherRequestHandler class
ast-grep --pattern 'class QueryPublisherRequestHandler {
$$$
executorManager.$$$
$$$
}'
Length of output: 95
Script:
#!/bin/bash
# Get the full content of the file to understand the class better
cat server/server/session/src/main/java/com/alipay/sofa/registry/server/session/remoting/console/handler/QueryPublisherRequestHandler.java
Length of output: 1844
Script:
#!/bin/bash
# Search for other potential usages of executorManager in different patterns
rg "executorManager" "server/server/session/src/main/java/com/alipay/sofa/registry/server/session/remoting/console/handler/QueryPublisherRequestHandler.java"
Length of output: 291
Script:
#!/bin/bash
# Check the parent class AbstractConsoleHandler
cat server/server/session/src/main/java/com/alipay/sofa/registry/server/session/remoting/console/handler/AbstractConsoleHandler.java
Length of output: 1657
Script:
#!/bin/bash
# Search for any references to executorManager in handler classes
rg -A 3 "executorManager" server/server/session/src/main/java/com/alipay/sofa/registry/server/session/remoting/console/handler/
Length of output: 2164
25-28: Consider distinguishing between valid and empty results.
The handler returns a success response even when no corresponding publishers are found. Depending on your business requirements, you might want to differentiate “no publishers found” from a truly successful scenario.
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/remoting/console/handler/QueryPublisherRequestHandlerTest.java (2)
34-44: Good setup for mocking Publishers.
The approach of creating multiple Publisher instances in a loop is concise and clear.
60-65: Verify the success response for non-existent dataInfoId.
When a dataInfoId is not found, the handler still returns a successful response with an empty list of publishers. Ensure that this behavior meets your functional requirements.
✅ Verification successful
Let me gather information about the handling of non-existent dataInfoId by examining the implementation and related tests.
Let me search for the implementation of the doHandle method to understand how non-existent dataInfoId cases are handled.
Based on the implementation, I can now provide the final response:
The success response for non-existent dataInfoId is the expected behavior
The handler is correctly implemented to return a successful response with an empty list when no publishers are found. This is a common pattern in REST APIs where querying for non-existent resources returns an empty result set rather than an error. The implementation in doHandle shows that it gets publishers using sessionDataStore.getDatas() and wraps the result (empty or not) in a successful GenericResponse without any conditional logic, which is the right approach for this use case.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for QueryPublisherRequestHandler implementation
ast-grep --pattern 'class QueryPublisherRequestHandler {
$$$
doHandle($$$) {
$$$
}
$$$
}'
# Search for related test cases
rg "QueryPublisherRequestHandler" -A 5
Length of output: 7332
Script:
#!/bin/bash
# Search for doHandle implementation in QueryPublisherRequestHandler
rg -A 20 "public Object doHandle" server/server/session/src/main/java/com/alipay/sofa/registry/server/session/remoting/console/handler/QueryPublisherRequestHandler.java
# Search for related GenericResponse usage
rg "GenericResponse" server/server/session/src/main/java/com/alipay/sofa/registry/server/session/remoting/console/handler/QueryPublisherRequestHandler.java
Length of output: 1152
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/PublisherUtils.java (1)
103-109: Check for potential null fields before constructing SimplePublisher.
Consider adding checks to ensure that the publisher and its source address are not null. If either is null, calling buildAddressString(), for instance, may trigger a NullPointerException.
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/SessionDigestResource.java (1)
22-28: Imports look good.
The newly introduced imports (PublisherUtils, QueryPublisherRequest, SimplePublisher) seem relevant. Ensure any unrelated imports are removed to keep dependencies lean.
|
|
||
| @GET | ||
| @Path("/data/zone/queryPublisher") | ||
| @Produces(MediaType.APPLICATION_JSON) | ||
| public GenericResponse<List<SimplePublisher>> queryZonePublisher(@QueryParam("dataInfoId") String dataInfoId) { | ||
| Collection<Publisher> publishers = this.sessionDataStore.getDatas(dataInfoId); | ||
| List<SimplePublisher> allPublishers = publishers.stream().map(PublisherUtils::convert).collect(Collectors.toList()); | ||
|
|
||
| List<URL> otherSessions = Sdks.getOtherConsoleServers(null, this.sessionServerConfig, this.metaNodeService); | ||
| if (!CollectionUtils.isEmpty(otherSessions)) { | ||
| Map<URL, CommonResponse> respMap = Sdks.concurrentSdkSend( | ||
| pubSubQueryZoneExecutor, | ||
| otherSessions, | ||
| (URL url) -> { | ||
| final QueryPublisherRequest req = new QueryPublisherRequest(dataInfoId); | ||
| return (CommonResponse) sessionConsoleExchanger.request(new SimpleRequest(req, url)).getResult(); | ||
| }, | ||
| 5000 | ||
| ); | ||
|
|
||
| for (Entry<URL, CommonResponse> entry : respMap.entrySet()) { | ||
| CommonResponse response = entry.getValue(); | ||
| if (response instanceof GenericResponse) { | ||
| GenericResponse<List<SimplePublisher>> genericResponse = (GenericResponse<List<SimplePublisher>>) response; | ||
| if (genericResponse.isSuccess()) { | ||
| List<SimplePublisher> subPublishers = genericResponse.getData(); | ||
| allPublishers.addAll(subPublishers); | ||
| } else { | ||
| LOGGER.error("url={} query publishers fail, response:{}.", | ||
| entry.getKey().getIpAddress(), entry.getValue()); | ||
| } | ||
| } else { | ||
| LOGGER.error("url={} query publishers fail, unexpect response type, response:{}.", | ||
| entry.getKey().getIpAddress(), entry.getValue()); | ||
| } | ||
| } | ||
| } | ||
| return new GenericResponse<List<SimplePublisher>>().fillSucceed(allPublishers); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate 'dataInfoId' parameter.
Before calling sessionDataStore.getDatas(dataInfoId), consider checking whether dataInfoId is null or empty. This will safeguard your logic and avoid passing invalid inputs to the data store.
+ if (StringUtils.isEmpty(dataInfoId)) {
+ return new GenericResponse<List<SimplePublisher>>()
+ .fillFailed("dataInfoId cannot be null or empty");
+ }
Collection<Publisher> publishers = this.sessionDataStore.getDatas(dataInfoId);
...📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @GET | |
| @Path("/data/zone/queryPublisher") | |
| @Produces(MediaType.APPLICATION_JSON) | |
| public GenericResponse<List<SimplePublisher>> queryZonePublisher(@QueryParam("dataInfoId") String dataInfoId) { | |
| Collection<Publisher> publishers = this.sessionDataStore.getDatas(dataInfoId); | |
| List<SimplePublisher> allPublishers = publishers.stream().map(PublisherUtils::convert).collect(Collectors.toList()); | |
| List<URL> otherSessions = Sdks.getOtherConsoleServers(null, this.sessionServerConfig, this.metaNodeService); | |
| if (!CollectionUtils.isEmpty(otherSessions)) { | |
| Map<URL, CommonResponse> respMap = Sdks.concurrentSdkSend( | |
| pubSubQueryZoneExecutor, | |
| otherSessions, | |
| (URL url) -> { | |
| final QueryPublisherRequest req = new QueryPublisherRequest(dataInfoId); | |
| return (CommonResponse) sessionConsoleExchanger.request(new SimpleRequest(req, url)).getResult(); | |
| }, | |
| 5000 | |
| ); | |
| for (Entry<URL, CommonResponse> entry : respMap.entrySet()) { | |
| CommonResponse response = entry.getValue(); | |
| if (response instanceof GenericResponse) { | |
| GenericResponse<List<SimplePublisher>> genericResponse = (GenericResponse<List<SimplePublisher>>) response; | |
| if (genericResponse.isSuccess()) { | |
| List<SimplePublisher> subPublishers = genericResponse.getData(); | |
| allPublishers.addAll(subPublishers); | |
| } else { | |
| LOGGER.error("url={} query publishers fail, response:{}.", | |
| entry.getKey().getIpAddress(), entry.getValue()); | |
| } | |
| } else { | |
| LOGGER.error("url={} query publishers fail, unexpect response type, response:{}.", | |
| entry.getKey().getIpAddress(), entry.getValue()); | |
| } | |
| } | |
| } | |
| return new GenericResponse<List<SimplePublisher>>().fillSucceed(allPublishers); | |
| } | |
| @GET | |
| @Path("/data/zone/queryPublisher") | |
| @Produces(MediaType.APPLICATION_JSON) | |
| public GenericResponse<List<SimplePublisher>> queryZonePublisher(@QueryParam("dataInfoId") String dataInfoId) { | |
| if (StringUtils.isEmpty(dataInfoId)) { | |
| return new GenericResponse<List<SimplePublisher>>() | |
| .fillFailed("dataInfoId cannot be null or empty"); | |
| } | |
| Collection<Publisher> publishers = this.sessionDataStore.getDatas(dataInfoId); | |
| List<SimplePublisher> allPublishers = publishers.stream().map(PublisherUtils::convert).collect(Collectors.toList()); | |
| List<URL> otherSessions = Sdks.getOtherConsoleServers(null, this.sessionServerConfig, this.metaNodeService); | |
| if (!CollectionUtils.isEmpty(otherSessions)) { | |
| Map<URL, CommonResponse> respMap = Sdks.concurrentSdkSend( | |
| pubSubQueryZoneExecutor, | |
| otherSessions, | |
| (URL url) -> { | |
| final QueryPublisherRequest req = new QueryPublisherRequest(dataInfoId); | |
| return (CommonResponse) sessionConsoleExchanger.request(new SimpleRequest(req, url)).getResult(); | |
| }, | |
| 5000 | |
| ); | |
| for (Entry<URL, CommonResponse> entry : respMap.entrySet()) { | |
| CommonResponse response = entry.getValue(); | |
| if (response instanceof GenericResponse) { | |
| GenericResponse<List<SimplePublisher>> genericResponse = (GenericResponse<List<SimplePublisher>>) response; | |
| if (genericResponse.isSuccess()) { | |
| List<SimplePublisher> subPublishers = genericResponse.getData(); | |
| allPublishers.addAll(subPublishers); | |
| } else { | |
| LOGGER.error("url={} query publishers fail, response:{}.", | |
| entry.getKey().getIpAddress(), entry.getValue()); | |
| } | |
| } else { | |
| LOGGER.error("url={} query publishers fail, unexpect response type, response:{}.", | |
| entry.getKey().getIpAddress(), entry.getValue()); | |
| } | |
| } | |
| } | |
| return new GenericResponse<List<SimplePublisher>>().fillSucceed(allPublishers); | |
| } |
hui-cha
force-pushed
the
feature/dataid_blacklist
branch
from
6d4a920 to
4071c28
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java (3)
43-56:⚠️ Potential issueAvoid catching
Throwable
CatchingThrowablecan mask critical errors likeOutOfMemoryError. Use narrower exception handling.-} catch (Throwable throwable) { - LOGGER.error("Save dataid black list exception", throwable); - return Result.failed("Save dataid black list exception"); -} +} catch (Exception e) { + LOGGER.error("Save dataid black list exception", e); + return Result.failed("Save dataid black list exception"); +}
58-71:⚠️ Potential issueAvoid catching
Throwable
This block has the same issue as the addBlackList method. CatchingThrowablecan hide significant system errors.-} catch (Throwable throwable) { - LOGGER.error("Delete dataid black list exception", throwable); - return Result.failed("Delete dataid black list exception"); -} +} catch (Exception e) { + LOGGER.error("Delete dataid black list exception", e); + return Result.failed("Delete dataid black list exception"); +}
160-162: 🛠️ Refactor suggestionImplement actual validation logic
This method returnstrueunconditionally and does not verify thedataInfocontent. Failing to validate may allow problematic or malicious DataInfo values.
🧹 Nitpick comments (6)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/filter/LeaderForwardFilter.java (2)
42-43: Consider constructor injection for better encapsulation
Field injection can make testing more challenging and hides dependencies. Constructor injection can make the dependencies of this class more explicit and help with immutability.
160-165: Optional: Replace setter injection with constructor injection
Currently,setMetaLeaderServiceis used for tests. If possible, injectingMetaLeaderServicedirectly via a constructor or using a testing framework’s capabilities can reduce potential for misconfiguration in production.server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/filter/LeaderForwardFilterTest.java (2)
36-78: Split into multiple test methods for clarity
The single test method exercises multiple distinct scenarios (first request, second request, get request). Splitting them up (e.g.,testFirstRequestNoForward,testSecondRequestForward, etc.) can improve readability and narrow down potential failures.
105-126: Consider concurrency effects
AmILeaderAnswertoggles states on consecutive calls, but it doesn’t handle concurrent test scenarios. If concurrency ever becomes relevant, you might need to ensure thread-safety or use concurrency testing tools.server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java (1)
95-103: Validate non-empty parameters
Before processing the blacklist, consider validating thatdataId,group, andinstanceIdare neither null nor empty. This protects against incomplete data.+if (StringUtils.isBlank(dataId)) { + return Result.failed("dataId cannot be null or empty"); +}server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java (1)
310-313: Consider making Filter bean conditional
If multiple filters implement similar leader-forward logic, conflicts may arise. A@ConditionalOnMissingBeanhelps preserve flexibility.-@Bean +@Bean +@ConditionalOnMissingBean public LeaderForwardFilter leaderForwardFilter() { ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java(4 hunks)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataInfoIDBlacklistResource.java(1 hunks)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/filter/LeaderForwardFilter.java(1 hunks)server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/filter/LeaderForwardRestController.java(1 hunks)server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/filter/LeaderForwardFilterTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/filter/LeaderForwardRestController.java
🔇 Additional comments (3)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/filter/LeaderForwardFilter.java (1)
45-49: Validate business logic for early return
When the current server is leader, the filter immediately returns without further checks. Verify whether there are any common actions or logging steps that should still happen in this scenario.
server/server/meta/src/test/java/com/alipay/sofa/registry/server/meta/resource/filter/LeaderForwardFilterTest.java (1)
38-62: Confirm request forwarding coverage
Ensure that the test validates key behaviors, such as headers and body transfer. Currently, verification stops at checking success codes and result messages. Consider asserting that internal forwarding logic was indeed called.
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java (1)
373-377: Confirm dependency injection for resource
DataInfoIDBlacklistResource depends on ProvideDataService and ProvideDataNotifier. Ensure they are wired automatically or consider constructor injection.
| private void proxyRequestToMetaLeader(ContainerRequestContext requestContext, String leaderAddr) { | ||
| HttpURLConnection connection = null; | ||
| try { | ||
| // 1. 获取请求信息 | ||
| String method = requestContext.getMethod(); | ||
| boolean hasEntity = requestContext.hasEntity(); | ||
| MultivaluedMap<String, String> headers = requestContext.getHeaders(); | ||
| UriInfo uriInfo = requestContext.getUriInfo(); | ||
| URI requestURI = uriInfo.getAbsolutePath(); | ||
| int requestPort = requestURI.getPort(); | ||
| String requestPath = requestURI.getRawPath(); | ||
|
|
||
| // 2. 拼接发送给 Meta Leader 的请求地址 | ||
| String newRequestURLStr = String.format("http://%s:%d%s", leaderAddr, requestPort, requestPath); | ||
| URL newRequestURL = new URL(newRequestURLStr); | ||
|
|
||
| // 3. 打开链接，这里因为协议写死是 HTTP 协议，所以拿到的必然是 HttpURLConnection | ||
| connection = (HttpURLConnection) newRequestURL.openConnection(); | ||
| connection.setRequestMethod(method); | ||
| if (hasEntity) { | ||
| connection.setDoOutput(true); | ||
| } | ||
| connection.setDoInput(true); | ||
|
|
||
| // 设置超时时间 | ||
| connection.setConnectTimeout((int) TimeUnit.SECONDS.toMillis(1)); | ||
| connection.setReadTimeout((int) TimeUnit.SECONDS.toMillis(3)); | ||
|
|
||
| // 设置请求头 | ||
| for (MultivaluedMap.Entry<String, List<String>> entry : headers.entrySet()) { | ||
| String headerKey = entry.getKey(); | ||
| List<String> headerValues = entry.getValue(); | ||
| if (CollectionUtils.isNotEmpty(headerValues)) { | ||
| connection.addRequestProperty(headerKey, headerValues.get(0)); | ||
| } | ||
| } | ||
|
|
||
| connection.connect(); | ||
|
|
||
| // 4. 发送请求，直接做一次拷贝 | ||
| if (hasEntity) { | ||
| try ( | ||
| OutputStream outputStream = connection.getOutputStream(); | ||
| InputStream inputStream = requestContext.getEntityStream() | ||
| ) { | ||
| IOUtils.copy(inputStream, outputStream); | ||
| } | ||
| } | ||
|
|
||
| // 5. 接收响应 | ||
| int responseCode = connection.getResponseCode(); | ||
| if (responseCode != HttpStatus.OK_200) { | ||
| LOGGER.error("Proxy request to meta leader fail, response code: {}, message: {}", responseCode, connection.getResponseMessage()); | ||
| Response response = | ||
| Response.status(Response.Status.INTERNAL_SERVER_ERROR) | ||
| .header("reason", "proxy request to meta leader fail: " + connection.getResponseMessage()) | ||
| .build(); | ||
| requestContext.abortWith(response); | ||
| return; | ||
| } | ||
|
|
||
| // 读取数据 | ||
| try ( | ||
| InputStream inputStream = connection.getInputStream(); | ||
| ByteArrayOutputStream outputStream = new ByteArrayOutputStream(1024); | ||
| ) { | ||
| IOUtils.copy(inputStream, outputStream); | ||
| byte[] responseData = outputStream.toByteArray(); | ||
| Response response = Response.ok(responseData).build(); | ||
| requestContext.abortWith(response); | ||
| } | ||
| } catch (Throwable throwable) { | ||
| LOGGER.error("Proxy request to meta leader exception, meta leader address: {}", leaderAddr, throwable); | ||
| Response response = | ||
| Response.status(Response.Status.INTERNAL_SERVER_ERROR) | ||
| .header("reason", "proxy request to meta leader exception") | ||
| .build(); | ||
| requestContext.abortWith(response); | ||
| } finally { | ||
| if (null != connection) { | ||
| try { | ||
| connection.disconnect(); | ||
| } catch (Throwable throwable) { | ||
| // 吃掉异常 | ||
| LOGGER.error("Disconnect connection to meta leader fail, meta leader address: {}", leaderAddr, throwable); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Forward remote status codes to preserve accuracy
Currently, if the leader response code is not 200, the filter aborts with a 500 status. This can obscure the actual status returned by the leader. Preserving the original status code and message may help debugging.
Limit catch scope to avoid suppressing critical errors
Catching Throwable can mask serious system errors (e.g., OutOfMemoryError). Prefer catching Exception to avoid suppressing critical runtime errors.
-} catch (Throwable throwable) {
+} catch (Exception e) {
LOGGER.error("Proxy request to meta leader exception, meta leader address: {}", leaderAddr, e);
...
}Committable suggestion skipped: line range outside the PR's diff.
Motivation:
增加一个根据 DataID 清理 Data 上的 Publisher 数据，以及忽略客户端发送 Publisher 的能力。这个能力主要提供给注册中心用于在不需要推动业务改造的情况下，自主清理掉某些数据量很大，但是没有业务使用的 Publisher 数据，以降低 Session 和 Data 的水位。
Modification:
主要改动是
1、Meta 增加了一些 API 用于增加和删除 DataID 黑名单，黑名单的数据目前是存放在 ProviderData 中，Key 是 session.datainfoid.blacklist#@#9600#@#CONFIG
2、Meta 上增加黑名单的时候可以通过扩展 DataInfoIDBlacklistResource 类来增加一些针对黑名单的过滤规则，以尽量规避对重要 DataID 的误操作
3、Session 上增加了一个新的 FetchSystemPropertyService，用于从 Meta 同步 DataID 黑名单
4、在 Session 感知到黑名单后，会主动清理掉当前 Session 的 Publisher，并通知 Data 清理到对应 Publisher 的数据
5、Session 上增加了一个新的 WrapperInterceptor，用于在客户端发送新的 Publisher 的时候进行拦截
6、针对新增内容增加了单测
Summary by CodeRabbit
New Features
Bug Fixes
Tests