Copilot commented on code in PR #17250:
URL: https://github.com/apache/iotdb/pull/17250#discussion_r2887989684
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/service/externalservice/ExternalServiceManagementService.java:
##########
@@ -417,6 +417,25 @@ public static ExternalServiceManagementService
getInstance() {
return ExternalServiceManagementServiceHolder.INSTANCE;
}
+ public void clearServiceUserCache(String userName) {
+ serviceInfos
+ .values()
+ .forEach(
+ serviceInfo -> {
+ // clear user cache of service with RUNNING state
+ if (serviceInfo.getState() == RUNNING) {
+ IExternalService serviceInstance =
serviceInfo.getServiceInstance();
+ // serviceInstance maybe null when an exception occurs during
the start of certain
+ // service in restoreRunningServiceInstance method
+ if (serviceInstance != null) {
+ // only clear user cache of the instance successfully
started and running
+ runWithServiceClassLoader(
+ serviceInfo, () ->
serviceInstance.clearUserCache(userName));
Review Comment:
If any external service's `clearUserCache` throws, the exception will
propagate through `runWithServiceClassLoader` and can break user/role cache
invalidation in the DataNode. It would be safer to catch `Throwable` (or at
least `Exception`) per service here, log it, and continue invalidating other
services and the local author cache.
```suggestion
try {
runWithServiceClassLoader(
serviceInfo, () ->
serviceInstance.clearUserCache(userName));
} catch (Throwable t) {
LOGGER.error(
"Failed to clear user cache for external service {}
and user {}.",
serviceInfo.getServiceName(),
userName,
t);
}
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/service/externalservice/ExternalServiceManagementService.java:
##########
@@ -417,6 +417,25 @@ public static ExternalServiceManagementService
getInstance() {
return ExternalServiceManagementServiceHolder.INSTANCE;
}
+ public void clearServiceUserCache(String userName) {
+ serviceInfos
+ .values()
+ .forEach(
+ serviceInfo -> {
+ // clear user cache of service with RUNNING state
+ if (serviceInfo.getState() == RUNNING) {
+ IExternalService serviceInstance =
serviceInfo.getServiceInstance();
+ // serviceInstance maybe null when an exception occurs during
the start of certain
+ // service in restoreRunningServiceInstance method
+ if (serviceInstance != null) {
+ // only clear user cache of the instance successfully
started and running
+ runWithServiceClassLoader(
+ serviceInfo, () ->
serviceInstance.clearUserCache(userName));
+ }
+ }
+ });
Review Comment:
`serviceInfos` is a `HashMap` annotated with `@GuardedBy("lock")`, but
`clearServiceUserCache` iterates it without acquiring the lock. This can race
with `createService/startService/stopService/dropService` and may throw
`ConcurrentModificationException` or observe inconsistent state. Consider
taking the read lock (or snapshotting `serviceInfos.values()` under the lock)
before iterating.
```suggestion
lock.readLock().lock();
try {
serviceInfos
.values()
.forEach(
serviceInfo -> {
// clear user cache of service with RUNNING state
if (serviceInfo.getState() == RUNNING) {
IExternalService serviceInstance =
serviceInfo.getServiceInstance();
// serviceInstance maybe null when an exception occurs
during the start of
// certain service in restoreRunningServiceInstance method
if (serviceInstance != null) {
// only clear user cache of the instance successfully
started and running
runWithServiceClassLoader(
serviceInfo, () ->
serviceInstance.clearUserCache(userName));
}
}
});
} finally {
lock.readLock().unlock();
}
```
##########
iotdb-api/external-service-api/src/main/java/org/apache/iotdb/externalservice/api/IExternalService.java:
##########
@@ -30,4 +30,7 @@ public interface IExternalService {
* guarantee to putBack thread or thread pool.
*/
void stop();
+
+ /** Clear user cache of current service. */
+ void clearUserCache(String userName);
Review Comment:
Adding an abstract method to `IExternalService` is a breaking API change for
any existing user-defined external service implementations (they will no longer
compile). If backward compatibility is desired, consider making this a
`default` no-op method in the interface so older implementations keep working
without modification.
```suggestion
default void clearUserCache(String userName) {}
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/auth/AuthorityChecker.java:
##########
@@ -130,6 +131,7 @@ public static IAuthorityFetcher getAuthorityFetcher() {
public static boolean invalidateCache(String username, String roleName) {
PipeInsertionDataNodeListener.getInstance().invalidateAllCache();
+
ExternalServiceManagementService.getInstance().clearServiceUserCache(username);
Review Comment:
`invalidateCache` now calls into the external service manager before
invalidating the DataNode author cache. If `clearServiceUserCache` fails (e.g.,
runtime exception from a user-defined service), this method will currently
propagate the error and skip invalidating the local cache. Consider wrapping
the external-service call in a try/catch and proceeding with local invalidation
even if external cache clearing fails.
```suggestion
try {
ExternalServiceManagementService.getInstance().clearServiceUserCache(username);
} catch (RuntimeException e) {
// Ignore failures from external service cache clearing to ensure
local cache is still invalidated.
}
```
##########
external-service-impl/rest/src/main/java/org/apache/iotdb/rest/protocol/filter/UserCache.java:
##########
@@ -51,6 +52,15 @@ public void setUser(String key, User user) {
cache.put(key, user);
}
+ public void clearUserCache(String userName) {
+ for (Map.Entry<String, User> mapValue : cache.asMap().entrySet()) {
+ if (mapValue.getValue().getUsername().equals(userName)) {
+ cache.invalidate(mapValue.getKey());
+ return;
+ }
+ }
+ }
Review Comment:
`clearUserCache` returns after invalidating the first cache entry for the
username, but the cache key is the full Authorization header (which can have
multiple distinct values per user, e.g., after password changes). To ensure
permission changes take effect consistently, invalidate *all* entries whose
cached `User` matches the username (e.g., collect matching keys and call
`invalidateAll`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
