imbajin commented on code in PR #2972:
URL: https://github.com/apache/hugegraph/pull/2972#discussion_r3105769382
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/LoadDetectFilter.java:
##########
@@ -83,7 +121,20 @@ public void filter(ContainerRequestContext context) {
long presumableFreeMem = (Runtime.getRuntime().maxMemory() -
allocatedMem) / Bytes.MB;
if (presumableFreeMem < minFreeMemory) {
- gcIfNeeded();
+ boolean shouldLog = this.allowRejectLog();
+ boolean gcTriggered = this.gcIfNeeded();
+ if (shouldLog) {
+ long allocatedMemAfterCheck =
Runtime.getRuntime().totalMemory() -
+
Runtime.getRuntime().freeMemory();
+ long recheckedFreeMem = (Runtime.getRuntime().maxMemory() -
+ allocatedMemAfterCheck) / Bytes.MB;
+ LOG.warn("Rejected request due to low free memory, method={},
path={}, " +
+ "presumableFreeMemMB={}, recheckedFreeMemMB={},
gcTriggered={}, " +
+ "minFreeMemoryMB={}",
+ context.getMethod(), context.getUriInfo().getPath(),
Review Comment:
‼️ **内存拒绝路径中的逻辑问题：GC 后未重新检查内存**
当前逻辑在 GC 后直接抛出异常，并没有重新检查内存是否已恢复到阈值以上。既然已经在这里做了 `recheckedFreeMem`
计算，建议更进一步：**GC 后重新检查内存，如果已恢复则放行请求**，减少不必要的 503。
当前行为：检测到内存不足 → 尝试 GC → 记录 GC 后的内存 → **仍然抛异常**
建议行为：检测到内存不足 → 尝试 GC → 重新检查 → 如果恢复则放行
```suggestion
boolean gcTriggered = this.gcIfNeeded();
if (gcTriggered) {
long allocatedMemAfterGc =
Runtime.getRuntime().totalMemory() -
Runtime.getRuntime().freeMemory();
long freeMemAfterGc = (Runtime.getRuntime().maxMemory() -
allocatedMemAfterGc) / Bytes.MB;
if (freeMemAfterGc >= minFreeMemory) {
this.logRejectWarning(
"Low memory recovered after GC, method={},
path={}, " +
"beforeFreeMB={}, afterFreeMB={}",
context.getMethod(),
context.getUriInfo().getPath(),
presumableFreeMem, freeMemAfterGc);
return;
}
}
this.logRejectWarning(
"Rejected request due to low free memory, method={},
path={}, " +
"presumableFreeMemMB={}, gcTriggered={},
minFreeMemoryMB={}",
context.getMethod(), context.getUriInfo().getPath(),
presumableFreeMem, gcTriggered, minFreeMemory);
```
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/LoadDetectFilter.java:
##########
@@ -54,11 +58,40 @@ public class LoadDetectFilter implements
ContainerRequestFilter {
private static final RateLimiter GC_RATE_LIMITER =
RateLimiter.create(1.0 / 30);
+ // Log at most 1 request per second to avoid too many logs when server is
under heavy load
+ private static final RateLimiter REJECT_LOG_RATE_LIMITER =
RateLimiter.create(1.0);
+
@Context
private jakarta.inject.Provider<HugeConfig> configProvider;
@Context
private jakarta.inject.Provider<WorkLoad> loadProvider;
+ public static boolean isWhiteAPI(ContainerRequestContext context) {
+ List<PathSegment> segments = context.getUriInfo().getPathSegments();
+ E.checkArgument(!segments.isEmpty(), "Invalid request uri '%s'",
+ context.getUriInfo().getPath());
+ String rootPath = segments.get(0).getPath();
Review Comment:
⚠️ **方法可见性变更：`gcIfNeeded` 从 `private static` 改为 `protected` 实例方法**
原方法是 `private static void gcIfNeeded()`，现改为 `protected boolean
gcIfNeeded()`。变更影响：
1. 扩大可见性（private → protected），破坏了封装性
2. 从静态方法变为实例方法
3. 主要目的似乎是为了测试时 override
建议保持 `private` 可见性，如果需要测试可测试性，可以：
- 使用 package-private 可见性 + `@VisibleForTesting` 注解
- 或者通过注入策略接口实现
同样的建议也适用于 `allowRejectLog()` 方法。
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/LoadDetectFilter.java:
##########
@@ -54,11 +58,40 @@ public class LoadDetectFilter implements
ContainerRequestFilter {
private static final RateLimiter GC_RATE_LIMITER =
RateLimiter.create(1.0 / 30);
Review Comment:
⚠️ **两条拒绝路径共享同一个 `REJECT_LOG_RATE_LIMITER`**
高负载拒绝和低内存拒绝共享同一个 RateLimiter（每秒 1 个 permit）。这意味着：
- 如果高负载拒绝消耗了 permit，紧接着的低内存拒绝就不会被记录（反之亦然）
- 运维人员在同时出现高负载和低内存时，可能只看到一种告警，遗漏另一种
建议为两种拒绝原因使用独立的 RateLimiter：
```java
private static final RateLimiter BUSY_LOG_LIMITER = RateLimiter.create(1.0);
private static final RateLimiter MEMORY_LOG_LIMITER =
RateLimiter.create(1.0);
```
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/LoadDetectFilter.java:
##########
@@ -54,11 +58,40 @@ public class LoadDetectFilter implements
ContainerRequestFilter {
private static final RateLimiter GC_RATE_LIMITER =
RateLimiter.create(1.0 / 30);
+ // Log at most 1 request per second to avoid too many logs when server is
under heavy load
+ private static final RateLimiter REJECT_LOG_RATE_LIMITER =
RateLimiter.create(1.0);
+
@Context
private jakarta.inject.Provider<HugeConfig> configProvider;
@Context
private jakarta.inject.Provider<WorkLoad> loadProvider;
+ public static boolean isWhiteAPI(ContainerRequestContext context) {
+ List<PathSegment> segments = context.getUriInfo().getPathSegments();
+ E.checkArgument(!segments.isEmpty(), "Invalid request uri '%s'",
+ context.getUriInfo().getPath());
+ String rootPath = segments.get(0).getPath();
+ return WHITE_API_LIST.contains(rootPath);
+ }
+
+ protected boolean gcIfNeeded() {
+ if (GC_RATE_LIMITER.tryAcquire(1)) {
+ System.gc();
+ return true;
+ }
+ return false;
+ }
+
+ protected boolean allowRejectLog() {
+ return REJECT_LOG_RATE_LIMITER.tryAcquire();
+ }
+
+ protected void logRejectWarning(String message, Object... args) {
Review Comment:
⚠️ **`logRejectWarning()` 与直接调用 `allowRejectLog()` 的不一致**
高负载路径使用封装的 `logRejectWarning()`，但低内存路径直接调用 `allowRejectLog()` + `LOG.warn()`：
```java
// 高负载路径
this.logRejectWarning("Rejected request due to high worker load...");
// 低内存路径
boolean shouldLog = this.allowRejectLog();
if (shouldLog) { LOG.warn(...); }
```
两条路径的日志方式不一致，降低了可读性。建议统一为同一种模式。另外 `logRejectWarning()`
在生产代码中只被调用了一次，是否真的需要抽取为独立方法值得考虑。
##########
hugegraph-server/hugegraph-api/src/main/java/org/apache/hugegraph/api/filter/LoadDetectFilter.java:
##########
@@ -83,7 +121,20 @@ public void filter(ContainerRequestContext context) {
long presumableFreeMem = (Runtime.getRuntime().maxMemory() -
allocatedMem) / Bytes.MB;
if (presumableFreeMem < minFreeMemory) {
- gcIfNeeded();
+ boolean shouldLog = this.allowRejectLog();
+ boolean gcTriggered = this.gcIfNeeded();
+ if (shouldLog) {
+ long allocatedMemAfterCheck =
Runtime.getRuntime().totalMemory() -
Review Comment:
🧹 **`recheckedFreeMem` 在 `gcTriggered=false` 时是冗余信息**
当 `gcTriggered=false` 时，`recheckedFreeMem` 和 `presumableFreeMem`
几乎完全相同（两次采样之间几乎没有时间差），日志中两个近似相同的值会造成困惑。
建议只在 `gcTriggered=true` 时才计算并记录 `recheckedFreeMem`，否则省略此字段。
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
