Skip to content

Conversation

@hui-cha
Copy link
Collaborator

@hui-cha hui-cha commented Sep 22, 2025

1、增加了一个自适应调整攒批配置的能力,并添加了查询攒批的 admin api
2、增加了一个 admin api 用来查询 meta 中 local datacenter 的值
3、调整版本号到 6.6.1

Summary by CodeRabbit

  • New Features

    • Automatic push-efficiency regulation with dynamic debouncing, adaptive limits and traffic-limit toggle.
    • New REST endpoints to fetch push debouncing settings, manage multi-cluster sync ignore IDs, list multi-cluster sync info, and query local data center.
    • Global gate to enable/disable client traffic operations; new configuration bean for push-efficiency.
  • Tests

    • Unit tests added for the regulator and configuration updater.
  • Chores

    • Project versions bumped to 6.6.1-auto-regulator across modules.

@coderabbitai
Copy link

coderabbitai bot commented Sep 22, 2025

Walkthrough

Multiple Maven parent/artifact versions bumped to 6.6.1-auto-regulator. Meta server: added custom session handler hook and DataCenter REST resource. Session server: introduced auto push-efficiency regulator, new config/beans/resources, traffic-operate flag, logging, and related tests.

Changes

Cohort / File(s) Summary
Version bumps (POMs)
pom.xml, core/pom.xml, client/pom.xml, client/all/pom.xml, client/api/pom.xml, client/impl/pom.xml, client/log/pom.xml, server/pom.xml, server/common/pom.xml, server/common/model/pom.xml, server/common/util/pom.xml, server/distribution/pom.xml, server/distribution/all/pom.xml, server/remoting/pom.xml, server/remoting/api/pom.xml, server/remoting/bolt/pom.xml, server/remoting/http/pom.xml, server/server/pom.xml, server/server/session/pom.xml, server/server/shared/pom.xml, server/server/meta/pom.xml, server/server/data/pom.xml, server/server/integration/pom.xml, server/store/pom.xml, server/store/api/pom.xml, server/store/jdbc/pom.xml, server/store/jraft/pom.xml, test/pom.xml
Parent/artifact versions updated from 6.6.06.6.1-auto-regulator. No other POM structural changes.
Meta server bootstrap/config/resources
server/server/meta/.../MetaServerBootstrap.java, .../MetaServerConfiguration.java, .../DataCenterResource.java, .../MultiClusterSyncResource.java
Added hook customSessionServerHandlers() and merged handler list; registered DataCenterResource bean; added DataCenterResource REST GET /datacenter/query; extended MultiClusterSyncResource with queryAll and add/remove ignoreDataInfoIds endpoints (auth/version checks, persistence).
Session: push-efficiency core
server/server/session/.../AutoPushEfficiencyConfig.java, .../AutoPushEfficiencyRegulator.java, .../ChangeDebouncingTime.java, .../PushEfficiencyImproveConfig.java, .../PushEfficiencyConfigUpdater.java
New auto-push-efficiency VO, regulator daemon, and updater component; PushEfficiencyImproveConfig now holds AutoPushEfficiencyConfig; updater manages lifecycle, regulator creation/closing, debouncing adjustments and traffic-limit updates.
Session: processors / services integration
server/server/session/.../ChangeProcessor.java, .../PushProcessor.java, .../FetchPushEfficiencyConfigService.java
ChangeProcessor gains per-center per-worker debouncing getters/setters; PushProcessor integrates optional regulator and increments push counts; FetchPushEfficiencyConfigService now depends on PushEfficiencyConfigUpdater (replacing previous trio of components).
Session REST/resources & config
server/server/session/.../PushEfficiencyConfigResource.java, .../ClientManagerResource.java, .../SessionServerConfiguration.java
Added PushEfficiencyConfigResource REST endpoint to fetch change debouncing times (token-auth); ClientManagerResource gains enableTrafficOperate flag with getters/setters and pre-checks that gate client on/off APIs; configuration registers push-related beans.
Logging
server/server/session/src/main/resources/log4j2.xml
Added appender and logger for AUTO-PUSH-EFFICIENCY-REGULATOR with daily rotation and 30-day retention; integrated into session logging.
Tests
server/server/session/src/test/java/.../FetchPushEfficiencyConfigServiceTest.java, .../push/AutoPushEfficiencyRegulatorTest.java, .../push/PushEfficiencyConfigUpdaterTest.java, .../store/SessionInterestsTest.java
Tests updated/added: Fetch test now injects PushEfficiencyConfigUpdater; new tests for regulator and updater behavior; one test file had formatting/import cleanup only.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Provider as ProviderData
  participant Fetch as FetchPushEfficiencyConfigService
  participant Updater as PushEfficiencyConfigUpdater
  participant Reg as AutoPushEfficiencyRegulator
  participant CP as ChangeProcessor
  participant PP as PushProcessor
  participant FS as FirePushService
  participant CMR as ClientManagerResource

  Provider->>Fetch: provide PushEfficiencyImproveConfig
  Fetch->>Updater: updateFromProviderData(config)
  alt auto enabled
    Updater->>Reg: create/replace regulator
    Updater->>CP: setChangeDebouncingMillis(...)
    Updater->>PP: setAutoPushEfficiencyRegulator(Reg)
    Updater->>FS: set regulator/delay
    Updater->>CMR: setEnableTrafficOperate(false)
  else auto disabled
    Updater->>Reg: close existing regulator (if any)
    Updater->>PP: clear regulator
    Updater->>CMR: setEnableTrafficOperate(false)
  end
Loading
sequenceDiagram
  autonumber
  actor Client as ClientAction
  participant PP as PushProcessor
  participant Reg as AutoPushEfficiencyRegulator
  participant Updater as PushEfficiencyConfigUpdater
  participant CP as ChangeProcessor
  participant CMR as ClientManagerResource

  Client->>PP: doPush(...)
  opt regulator present
    PP->>Reg: safeIncrementPushCount()
    Reg->>Reg: evaluate windows & thresholds
    alt need adjust
      Reg->>Updater: updateDebouncingTime(t, max)
      Updater->>CP: setChangeDebouncingMillis(t, max)
      Reg->>Updater: updateTrafficOperateLimitSwitch(flag)
      Updater->>CMR: setEnableTrafficOperate(!flag)
    end
  end
  PP-->>Client: push proceeds
Loading
sequenceDiagram
  autonumber
  actor Caller as API Client
  participant Res as PushEfficiencyConfigResource
  participant Auth as AuthChecker
  participant CP as ChangeProcessor

  Caller->>Res: GET /api/push/efficiency/getChangeDebouncingMillis (token)
  Res->>Auth: check(token)
  alt auth fail
    Res-->>Caller: GenericResponse.fail
  else auth ok
    Res->>CP: getChangeDebouncingMillis()
    CP-->>Res: Map<center, ChangeDebouncingTime[]>
    Res-->>Caller: GenericResponse.success(data)
  end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

I nudge the gears with whiskered grace,
New tags hop in every place.
I count the pushes, time the clap,
Debounce the noise, flip the map.
Logs spin softly—hop, compile, embrace. 🐇

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 7.69% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Feature/do push optimization" refers directly to the primary work in this changeset — adding adaptive push/batch-accumulation optimization and related admin APIs — and therefore describes a real and central aspect of the PR; the phrasing is slightly non-idiomatic but not misleading.
✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 680d416 and 5a73faa.

📒 Files selected for processing (2)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java (1 hunks)
  • server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build
  • GitHub Check: build
🔇 Additional comments (5)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java (5)

77-119: LGTM! Configuration management logic is well-structured.

The logic correctly handles both auto push efficiency enablement and disablement cases, properly closing old regulators before creating new ones, and nullifying references when disabled. The initial configuration adjustments for debouncing time minimums are also appropriate.


138-149: LGTM! Guard conditions properly prevent configuration drift.

Both methods correctly check useAutoPushEfficiency to ensure updates only apply when auto push efficiency is active, preventing configuration drift from provider data.

Also applies to: 151-164


195-219: LGTM! Testing support methods are appropriately scoped.

The @VisibleForTesting annotations properly mark these methods as test-only, and the accessors/mutators provide necessary testing capabilities without exposing implementation details to production code.


166-193: This implements the SmartLifecycle contract incorrectly and represents a critical bug that was previously flagged.

Spring checks isRunning() to determine if a SmartLifecycle bean is active during shutdown, but this implementation returns stop instead of !stop in Line 189. Additionally, the default implementation for isAutoStartup() returns true, yet start() is a no-op that doesn't initialize the component. Finally, the stop() method doesn't clear stale references after closing the regulator.

The implementation violates Spring's lifecycle contract:

  • isRunning() returns stop instead of !stop (inverted logic)
  • start() should reset stop=false and initialize the component
  • stop() should null out the regulator and detach it from PushProcessor to prevent stale references

Apply this diff to fix the lifecycle contract:

   @Override
-  public void start() {}
+  public void start() {
+    this.lock.lock();
+    try {
+      this.stop = false;
+    } finally {
+      this.lock.unlock();
+    }
+  }

   @Override
   public void stop() {
     // Bean 被销毁的时候需要清理释放线程资源
     this.lock.lock();
     try {
       if (!this.stop) {
         this.stop = true;
         if (null != this.autoPushEfficiencyRegulator) {
           this.autoPushEfficiencyRegulator.close();
+          this.autoPushEfficiencyRegulator = null;
         }
+        this.useAutoPushEfficiency = false;
+        this.pushProcessor.setAutoPushEfficiencyRegulator(null);
       }
     } finally {
       this.lock.unlock();
     }
   }

   @Override
   public boolean isRunning() {
     this.lock.lock();
     try {
-      return this.stop;
+      return !this.stop;
     } finally {
       this.lock.unlock();
     }
   }

132-132: No change needed — boolean semantics are correct.

enableTrafficOperate == true allows operations (disables rate limiting); updateTrafficOperateLimitSwitch inverts trafficOperateLimitSwitch and calls setEnableTrafficOperate(!trafficOperateLimitSwitch), so the setEnableTrafficOperate(true) at line 132 correctly disables limiting.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (35)
server/common/util/pom.xml (1)

106-116: Duplicate dependency: powermock-api-mockito is declared twice.

Maven will ignore the duplicate, but keeping both is noise and can confuse dependency tooling.

Apply:

         <dependency>
           <groupId>org.powermock</groupId>
           <artifactId>powermock-api-mockito</artifactId>
         </dependency>
-        <dependency>
-          <groupId>org.powermock</groupId>
-          <artifactId>powermock-api-mockito</artifactId>
-        </dependency>
server/store/jdbc/pom.xml (1)

9-9: Parent bump is correct; proceed.

Minor follow-up: tests pin H2 to 1.4.200. If feasible, consider moving to 2.x later to pick up fixes, but not blocking this PR.

Optional check for remaining 6.6.0 parents with the repo-wide script shared earlier.

server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.java (1)

219-221: Tests look good; flip Assert argument order for clearer failures

Use Assert.assertEquals(expected, actual). It’s currently reversed in a few spots.

-    Assert.assertEquals(subscribersResult1.size(), appOneNum);
+    Assert.assertEquals(appOneNum, subscribersResult1.size());

-    Assert.assertEquals(subscribersResult2.size(), 1);
+    Assert.assertEquals(1, subscribersResult2.size());

-    Assert.assertEquals(subscribersResult3.size(), appOneNum + appTwoNum);
+    Assert.assertEquals(appOneNum + appTwoNum, subscribersResult3.size());

Also applies to: 228-231, 237-240, 245-247, 257-259, 267-268, 270-276

server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataCenterResource.java (2)

38-41: Make logger static final

Follow project logging conventions; avoid per-instance logger.

-  private Logger LOGGER = LoggerFactory.getLogger(DataCenterResource.class);
+  private static final Logger LOG = LoggerFactory.getLogger(DataCenterResource.class);

42-55: Rename method and standardize response type

Method name is misleading; also prefer GenericResponse to align with other resources and return data in ‘data’ instead of ‘message’.

-  @GET
-  @Path("query")
-  @Produces(MediaType.APPLICATION_JSON)
-  public Result queryBlackList() {
+  @GET
+  @Path("query")
+  @Produces(MediaType.APPLICATION_JSON)
+  public GenericResponse<String> queryLocalDataCenter() {
     try {
       String localDataCenter = this.metaServerConfig.getLocalDataCenter();
-      Result result = Result.success();
-      result.setMessage(localDataCenter);
-      return result;
+      return new GenericResponse<String>().fillSucceed(localDataCenter);
     } catch (Throwable throwable) {
-      LOGGER.error("Query meta local datacenter exception", throwable);
-      return Result.failed("Query meta local datacenter exception");
+      LOG.error("Query meta local datacenter exception", throwable);
+      return new GenericResponse<String>().fillFailed("Query meta local datacenter exception");
     }
   }

Additional import updates required:

// replace Result with GenericResponse
- import com.alipay.sofa.registry.core.model.Result;
+ import com.alipay.sofa.registry.common.model.GenericResponse;
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerBootstrap.java (1)

239-246: Avoid double method call and external CollectionUtils dependency

Use local variable once and simple null/empty checks; drop org.apache.commons.collections.CollectionUtils here.

-        List<AbstractServerHandler> mergedSessionServerHandlers =
-            new ArrayList<>(this.sessionServerHandlers);
-
-        Collection<AbstractServerHandler> customSessionServerHandlers =
-            this.customSessionServerHandlers();
-        if (CollectionUtils.isNotEmpty(customSessionServerHandlers)) {
-          mergedSessionServerHandlers.addAll(this.customSessionServerHandlers());
-        }
+        List<AbstractServerHandler> mergedSessionServerHandlers =
+            new ArrayList<>(sessionServerHandlers);
+        Collection<AbstractServerHandler> customHandlers = customSessionServerHandlers();
+        if (customHandlers != null && !customHandlers.isEmpty()) {
+          mergedSessionServerHandlers.addAll(customHandlers);
+        }
 
         sessionServer =
           ...
-                mergedSessionServerHandlers.toArray(
-                    new ChannelHandler[mergedSessionServerHandlers.size()]));
+                mergedSessionServerHandlers.toArray(new ChannelHandler[mergedSessionServerHandlers.size()]));
 
 ...
   protected Collection<AbstractServerHandler> customSessionServerHandlers() {
     return Collections.emptyList();
   }

Also applies to: 252-253, 269-271

server/server/session/src/main/resources/log4j2.xml (1)

844-857: Appender OK; consider size-based rollover too

Add SizeBasedTriggeringPolicy to cap file growth under bursts.

         <Policies>
             <TimeBasedTriggeringPolicy/>
+            <SizeBasedTriggeringPolicy size="256MB"/>
         </Policies>
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/MultiClusterSyncResource.java (3)

65-73: Type the GenericResponse to avoid rawtype warnings

Minor generics fix.

-  public GenericResponse<Set<MultiClusterSyncInfo>> queryAll() {
-    GenericResponse<Set<MultiClusterSyncInfo>> response = new GenericResponse();
+  public GenericResponse<Set<MultiClusterSyncInfo>> queryAll() {
+    GenericResponse<Set<MultiClusterSyncInfo>> response = new GenericResponse<>();

527-573: Harden input parsing and version validation for ignoreDataInfoIds/add

  • Trim/omit empty CSV items.
  • Validate expectVersion is numeric before parse to prevent 500s.
-    if (StringUtils.isBlank(remoteDataCenter)
-        || StringUtils.isBlank(ignoreDataInfoIds)
-        || StringUtils.isBlank(expectVersion)) {
+    if (StringUtils.isBlank(remoteDataCenter)
+        || StringUtils.isBlank(ignoreDataInfoIds)
+        || StringUtils.isBlank(expectVersion)) {
       return CommonResponse.buildFailedResponse(
           "remoteDataCenter, ignoreDataInfoIds, expectVersion is not allow empty.");
     }
 
+    if (!NumberUtils.isDigits(expectVersion)) {
+      return CommonResponse.buildFailedResponse("expectVersion must be numeric.");
+    }
+
     MultiClusterSyncInfo exist = multiClusterSyncRepository.query(remoteDataCenter);
 
-    if (exist == null || exist.getDataVersion() != Long.parseLong(expectVersion)) {
+    if (exist == null || exist.getDataVersion() != NumberUtils.toLong(expectVersion)) {
       return CommonResponse.buildFailedResponse(
           StringFormatter.format(
               "remoteDataCenter:{}, expectVersion:{} not exist.", remoteDataCenter, expectVersion));
     }
 
-    exist.getIgnoreDataInfoIds().addAll(Sets.newHashSet(ignoreDataInfoIds.split(",")));
+    exist.getIgnoreDataInfoIds()
+        .addAll(Sets.newHashSet(Splitter.on(',').trimResults().omitEmptyStrings().split(ignoreDataInfoIds)));

Add import (outside hunk):

import com.google.common.base.Splitter;

575-620: Mirror the same hardening for ignoreDataInfoIds/remove

Trim/omit empty items and validate expectVersion.

-    if (StringUtils.isBlank(remoteDataCenter)
-        || StringUtils.isBlank(ignoreDataInfoIds)
-        || StringUtils.isBlank(expectVersion)) {
+    if (StringUtils.isBlank(remoteDataCenter)
+        || StringUtils.isBlank(ignoreDataInfoIds)
+        || StringUtils.isBlank(expectVersion)) {
       return CommonResponse.buildFailedResponse(
           "remoteDataCenter, ignoreDataInfoIds, expectVersion is not allow empty.");
     }
 
+    if (!NumberUtils.isDigits(expectVersion)) {
+      return CommonResponse.buildFailedResponse("expectVersion must be numeric.");
+    }
+
     MultiClusterSyncInfo exist = multiClusterSyncRepository.query(remoteDataCenter);
 
-    if (exist == null || exist.getDataVersion() != Long.parseLong(expectVersion)) {
+    if (exist == null || exist.getDataVersion() != NumberUtils.toLong(expectVersion)) {
       return CommonResponse.buildFailedResponse(
           StringFormatter.format(
               "remoteDataCenter:{}, expectVersion:{} not exist.", remoteDataCenter, expectVersion));
     }
 
-    exist.getIgnoreDataInfoIds().removeAll(Sets.newHashSet(ignoreDataInfoIds.split(",")));
+    exist.getIgnoreDataInfoIds()
+        .removeAll(Sets.newHashSet(Splitter.on(',').trimResults().omitEmptyStrings().split(ignoreDataInfoIds)));

Add import (outside hunk):

import com.google.common.base.Splitter;
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/ChangeProcessor.java (2)

26-26: Avoid wildcard import.

Prefer explicit imports to keep surface area clear and avoid accidental type shadowing.


153-157: Setter OK, but consider enforcing invariants.

Guard against negative values and ensure max >= base at the boundary as well.

   public void setChangeDebouncingMillis(
       int changeDebouncingMillis, int changeDebouncingMaxMillis) {
-    this.changeDebouncingMillis = changeDebouncingMillis;
-    this.changeDebouncingMaxMillis = changeDebouncingMaxMillis;
+    if (changeDebouncingMillis < 0) {
+      changeDebouncingMillis = 0;
+    }
+    if (changeDebouncingMaxMillis < changeDebouncingMillis) {
+      changeDebouncingMaxMillis = changeDebouncingMillis;
+    }
+    this.changeDebouncingMillis = changeDebouncingMillis;
+    this.changeDebouncingMaxMillis = changeDebouncingMaxMillis;
   }
server/server/session/pom.xml (1)

41-42: Duplicate dependency: registry-server-shared appears twice.

Remove the duplicate to avoid resolution ambiguity.

         <dependency>
             <groupId>com.alipay.sofa</groupId>
             <artifactId>registry-server-shared</artifactId>
         </dependency>
@@
-        <dependency>
-            <groupId>com.alipay.sofa</groupId>
-            <artifactId>registry-server-shared</artifactId>
-        </dependency>

Also applies to: 73-74

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushProcessor.java (1)

350-356: Counting pushes before dispatch: confirm desired semantics re: retries.

This increments on every attempt (including retries). If regulation should reflect unique pushes rather than attempts, consider gating on first attempt per PushingTaskKey.

As a minimal guard, you could increment only when no existing record is present (attempt-1):

-      if (null != currentAutoPushEfficiencyRegulator) {
-        currentAutoPushEfficiencyRegulator.safeIncrementPushCount();
-      }
+      if (null != currentAutoPushEfficiencyRegulator
+          && !pushingRecords.containsKey(task.pushingTaskKey)) {
+        currentAutoPushEfficiencyRegulator.safeIncrementPushCount();
+      }

Is the regulator intended to react to attempts or distinct pushes? If attempts, ignore this suggestion.

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyImproveConfig.java (1)

287-287: Avoid reflectionToString in logs to limit noise/sensitivity.

Consider a concise style or a manual toString to avoid dumping large sets/IPs.

-    return ToStringBuilder.reflectionToString(this);
+    return ToStringBuilder.reflectionToString(this, org.apache.commons.lang.builder.ToStringStyle.SHORT_PREFIX_STYLE);
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java (2)

48-70: Prefer explicit imports over wildcards for maintainability.

Wildcard imports make refactors and static analysis harder.

Also applies to: 58-61, 67-70


501-505: Add @ConditionalOnMissingBean to PushEfficiencyConfigResource bean

Allow overriding the bean; PushEfficiencyConfigResource is JAX‑RS annotated (server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/PushEfficiencyConfigResource.java — @path("api/push/efficiency")).

-    @Bean
+    @Bean
+    @ConditionalOnMissingBean
     public PushEfficiencyConfigResource pushEfficiencyConfigResource() {
       return new PushEfficiencyConfigResource();
     }

File to change: server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java (around lines 501-505).

server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java (2)

31-39: Add interaction verifications and guaranteed cleanup.

Verify updater propagates the regulator to PushProcessor, and ensure stop() always runs.

@@
-  public void testUpdateFromProviderData() {
+  public void testUpdateFromProviderData() {
@@
-    PushEfficiencyConfigUpdater pushEfficiencyConfigUpdater = new PushEfficiencyConfigUpdater();
+    PushEfficiencyConfigUpdater pushEfficiencyConfigUpdater = new PushEfficiencyConfigUpdater();
     pushEfficiencyConfigUpdater.setChangeProcessor(changeProcessor);
     pushEfficiencyConfigUpdater.setPushProcessor(pushProcessor);
     pushEfficiencyConfigUpdater.setFirePushService(firePushService);
+    try {
@@
-    // 清理释放线程资源
-    pushEfficiencyConfigUpdater.stop();
+    // 清理释放线程资源
+    pushEfficiencyConfigUpdater.stop();
+    } finally {
+      // 保底释放
+      pushEfficiencyConfigUpdater.stop();
+    }

Optionally verify propagation:

+    Mockito.verify(pushProcessor, Mockito.atLeastOnce()).setAutoPushEfficiencyRegulator(Mockito.any());

82-91: Window assertions OK; consider asserting debouncing flags too.

Add checks for isEnableDebouncingTime and isEnableMaxDebouncingTime if exposed.

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/PushEfficiencyConfigResource.java (4)

40-40: Use static final logger.

Make the logger constant and class‑scoped.

-  private Logger LOGGER = LoggerFactory.getLogger(PushEfficiencyConfigResource.class);
+  private static final Logger LOGGER =
+      LoggerFactory.getLogger(PushEfficiencyConfigResource.class);

57-65: Use generic type for GenericResponse and avoid catching Throwable.

Raw types lose type safety; catching Throwable is too broad.

-      Map<String, ChangeDebouncingTime[]> changeDebouncingTimes =
-          this.changeProcessor.getChangeDebouncingMillis();
-      return new GenericResponse().fillSucceed(changeDebouncingTimes);
-    } catch (Throwable throwable) {
+      Map<String, ChangeDebouncingTime[]> changeDebouncingTimes =
+          this.changeProcessor.getChangeDebouncingMillis();
+      return new GenericResponse<Map<String, ChangeDebouncingTime[]>>()
+          .fillSucceed(changeDebouncingTimes);
+    } catch (Exception e) {
       LOGGER.error(
           "[module=PushEfficiencyConfigResource][method=getChangeDebouncingMillis] getChangeDebouncingMillis exception",
-          throwable);
-      return new GenericResponse().fillFailed("getChangeDebouncingMillis exception");
+          e);
+      return new GenericResponse<Map<String, ChangeDebouncingTime[]>>()
+          .fillFailed("getChangeDebouncingMillis exception");

44-49: Consider constructor injection over field injection.

Constructor injection makes dependencies explicit and eases testing.


50-51: Auth model relies on a hard-coded shared secret.

AuthChecker uses a fixed token. Externalize to config, support rotation, and (optionally) restrict by source IP/role.

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyRegulator.java (3)

127-129: Avoid starting threads in the constructor.

Starting the daemon in the constructor causes this to escape before full construction and complicates lifecycle control. Expose an explicit start() and let the owner manage it.

-    // 启动定时任务
-    ConcurrentUtils.createDaemonThread("AutoPushEfficiencyRegulator-" + this.id, this).start();
+    // 由外部显式调用 start() 管理生命周期

Additional method (outside this hunk):

public void start() {
  ConcurrentUtils.createDaemonThread("AutoPushEfficiencyRegulator-" + this.id, this).start();
}

227-251: Micro-optimization: avoid querying load average when unnecessary.

Skip getSystemLoadAverage() when push isn’t high and the switch is already off.

   private void tryUpdateTrafficOperateLimitSwitch(boolean pushCountIsHigh) {
     if (!this.trafficOperateLimitSwitch.isEnable()) {
       // 如果没有开启支持操作开关流,那么就不执行后续的代码了,尽量尝试避免获取系统负载
       return;
     }
+    // 当推送频率不高且开关已关闭时,无需读取系统负载
+    if (!pushCountIsHigh && !this.trafficOperateLimitSwitch.load()) {
+      return;
+    }

335-342: Initial value for IntMetric may be surprising.

Starting at min immediately affects config on first update; consider initializing to defaultV to preserve current behavior when auto mode first turns on.

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyConfig.java (1)

34-45: Add min/max/step sanity checks (optional).

Validate min ≤ max and step > 0 to avoid silent no‑ops.

Also applies to: 72-82

server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyRegulatorTest.java (2)

67-71: Assertion order: expected before actual.

Switch parameters to improve failure messages.

-      Assert.assertEquals(
-          debouncingTime, 1000 /*AutoPushEfficiencyConfig.DEFAULT_DEBOUNCING_TIME_MAX*/);
-      Assert.assertEquals(
-          maxDebouncingTime, 3000 /*AutoPushEfficiencyConfig.DEFAULT_MAX_DEBOUNCING_TIME_MAX*/);
+      Assert.assertEquals(
+          1000 /*AutoPushEfficiencyConfig.DEFAULT_DEBOUNCING_TIME_MAX*/, debouncingTime);
+      Assert.assertEquals(
+          3000 /*AutoPushEfficiencyConfig.DEFAULT_MAX_DEBOUNCING_TIME_MAX*/, maxDebouncingTime);
@@
-      Assert.assertEquals(
-          debouncingTime, 100 /*AutoPushEfficiencyConfig.DEFAULT_DEBOUNCING_TIME_MIN*/);
-      Assert.assertEquals(
-          maxDebouncingTime, 1000 /*AutoPushEfficiencyConfig.DEFAULT_MAX_DEBOUNCING_TIME_MIN*/);
+      Assert.assertEquals(
+          100 /*AutoPushEfficiencyConfig.DEFAULT_DEBOUNCING_TIME_MIN*/, debouncingTime);
+      Assert.assertEquals(
+          1000 /*AutoPushEfficiencyConfig.DEFAULT_MAX_DEBOUNCING_TIME_MIN*/, maxDebouncingTime);

Also applies to: 76-80, 129-134, 139-143


51-53: Prefer stubbing only target methods instead of global default Answer.

Limit Answer to updateDebouncingTime to avoid accidental coupling with other calls.

Example (outside current hunk):

PushEfficiencyConfigUpdater updater = Mockito.mock(PushEfficiencyConfigUpdater.class);
Mockito.doAnswer(mockPushEfficiencyConfigUpdater)
    .when(updater).updateDebouncingTime(Mockito.anyInt(), Mockito.anyInt());

Also applies to: 115-118

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchPushEfficiencyConfigService.java (2)

24-24: Prefer constructor injection over field injection for required deps

Constructor injection makes required dependencies explicit, avoids accidental nulls, and improves testability.

Also applies to: 44-45


103-107: Null‑guard the testing setter

Guard against accidental nulls in tests to avoid NPEs later.

Apply this diff:

   public FetchPushEfficiencyConfigService setPushEfficiencyConfigUpdater(
       PushEfficiencyConfigUpdater pushEfficiencyConfigUpdater) {
-    this.pushEfficiencyConfigUpdater = pushEfficiencyConfigUpdater;
+    if (pushEfficiencyConfigUpdater == null) {
+      throw new IllegalArgumentException("pushEfficiencyConfigUpdater must not be null");
+    }
+    this.pushEfficiencyConfigUpdater = pushEfficiencyConfigUpdater;
     return this;
   }
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java (4)

64-70: Add null‑guard on input config

Avoid NPEs if callers pass null.

Apply this diff:

   public void updateFromProviderData(PushEfficiencyImproveConfig pushEfficiencyImproveConfig) {
+    if (pushEfficiencyImproveConfig == null) {
+      LOGGER.warn("[PushEfficiencyConfigUpdater] ignore null config");
+      return;
+    }

83-90: Guard regulator re-creation with try/catch

If closing the old regulator or constructing the new one throws, we can end up with no regulator and partial state. Recommend catching and logging, leaving previous state intact on failure.

Also applies to: 104-118


38-45: Switch to constructor injection for required collaborators

ChangeProcessor, PushProcessor, FirePushService, and ClientManagerResource are required. Prefer constructor injection and mark fields final for stronger guarantees and easier testing.

Also applies to: 199-212


46-57: Make the lock final

lock doesn’t need reassignment; mark it final and initialize at declaration.

Apply this diff:

-  private Lock lock;
+  private final Lock lock = new ReentrantLock();
 
-  public PushEfficiencyConfigUpdater() {
-    this.lock = new ReentrantLock();
-    this.stop = false;
-  }
+  public PushEfficiencyConfigUpdater() {
+    this.stop = false;
+  }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac38b85 and 693e798.

📒 Files selected for processing (48)
  • client/all/pom.xml (1 hunks)
  • client/api/pom.xml (1 hunks)
  • client/impl/pom.xml (1 hunks)
  • client/log/pom.xml (1 hunks)
  • client/pom.xml (1 hunks)
  • core/pom.xml (1 hunks)
  • pom.xml (1 hunks)
  • server/common/model/pom.xml (1 hunks)
  • server/common/pom.xml (1 hunks)
  • server/common/util/pom.xml (1 hunks)
  • server/distribution/all/pom.xml (1 hunks)
  • server/distribution/pom.xml (1 hunks)
  • server/pom.xml (1 hunks)
  • server/remoting/api/pom.xml (1 hunks)
  • server/remoting/bolt/pom.xml (1 hunks)
  • server/remoting/http/pom.xml (1 hunks)
  • server/remoting/pom.xml (1 hunks)
  • server/server/data/pom.xml (1 hunks)
  • server/server/integration/pom.xml (1 hunks)
  • server/server/meta/pom.xml (1 hunks)
  • server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerBootstrap.java (3 hunks)
  • server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java (1 hunks)
  • server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataCenterResource.java (1 hunks)
  • server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/MultiClusterSyncResource.java (3 hunks)
  • server/server/pom.xml (1 hunks)
  • server/server/session/pom.xml (1 hunks)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java (3 hunks)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchPushEfficiencyConfigService.java (4 hunks)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyConfig.java (1 hunks)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyRegulator.java (1 hunks)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/ChangeDebouncingTime.java (1 hunks)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/ChangeProcessor.java (4 hunks)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java (1 hunks)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyImproveConfig.java (4 hunks)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushProcessor.java (3 hunks)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/ClientManagerResource.java (6 hunks)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/PushEfficiencyConfigResource.java (1 hunks)
  • server/server/session/src/main/resources/log4j2.xml (2 hunks)
  • server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchPushEfficiencyConfigServiceTest.java (2 hunks)
  • server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyRegulatorTest.java (1 hunks)
  • server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java (1 hunks)
  • server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.java (4 hunks)
  • server/server/shared/pom.xml (1 hunks)
  • server/store/api/pom.xml (1 hunks)
  • server/store/jdbc/pom.xml (1 hunks)
  • server/store/jraft/pom.xml (1 hunks)
  • server/store/pom.xml (1 hunks)
  • test/pom.xml (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.java (1)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/store/DataInfo.java (1)
  • DataInfo (29-180)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/PushEfficiencyConfigResource.java (4)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/GenericResponse.java (1)
  • GenericResponse (25-86)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/ChangeDebouncingTime.java (1)
  • ChangeDebouncingTime (23-51)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/ChangeProcessor.java (1)
  • ChangeProcessor (30-284)
server/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/resource/AuthChecker.java (1)
  • AuthChecker (25-32)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/ClientManagerResource.java (1)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/CommonResponse.java (1)
  • CommonResponse (27-123)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/MultiClusterSyncResource.java (5)
server/server/shared/src/main/java/com/alipay/sofa/registry/server/shared/resource/AuthChecker.java (1)
  • AuthChecker (25-32)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/GenericResponse.java (1)
  • GenericResponse (25-86)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/CommonResponse.java (1)
  • CommonResponse (27-123)
server/common/util/src/main/java/com/alipay/sofa/registry/util/StringFormatter.java (1)
  • StringFormatter (22-29)
server/common/model/src/main/java/com/alipay/sofa/registry/common/model/console/PersistenceDataBuilder.java (1)
  • PersistenceDataBuilder (25-57)
server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataCenterResource.java (1)
core/src/main/java/com/alipay/sofa/registry/core/model/Result.java (1)
  • Result (25-93)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyRegulator.java (3)
server/common/util/src/main/java/com/alipay/sofa/registry/log/LoggerFactory.java (1)
  • LoggerFactory (23-66)
server/common/util/src/main/java/com/alipay/sofa/registry/util/ConcurrentUtils.java (1)
  • ConcurrentUtils (34-122)
server/common/util/src/main/java/com/alipay/sofa/registry/util/LoopRunnable.java (1)
  • LoopRunnable (27-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build
  • GitHub Check: build
  • GitHub Check: build
🔇 Additional comments (46)
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/ClientManagerResource.java (3)

81-81: LGTM! Thread-safe flag for traffic operations.

The volatile boolean is appropriate for a simple flag that controls thread behavior, as volatile provides visibility guarantees for single-read and single-write operations on variables like boolean. The default value of true enables traffic operations by default, which is a sensible default behavior.


96-99: LGTM! Consistent traffic operation gating across endpoints.

The implementation correctly gates traffic operations by checking enableTrafficOperate before proceeding with the actual operations. The volatile flag ensures that changes are immediately visible to all threads, preventing thread visibility issues. The consistent error message "too many request" across all endpoints provides a uniform user experience, though it reads like a rate limiting message which aligns with the Chinese comment indicating this is for flow control (限流).

Also applies to: 121-124, 146-149, 188-191


268-274: LGTM! Standard getter and setter methods.

The accessor methods follow Java conventions and maintain thread safety by operating on the volatile field. The getter returns the current state, while the setter allows external control of the traffic operation flag.

server/store/jraft/pom.xml (1)

9-9: Aligned with new parent; LGTM.

Please include this module in the repo-wide version sweep to avoid mixed parents during release.

Use the same script provided in the remoting-bolt comment to verify no 6.6.0 remnants.

server/store/api/pom.xml (1)

8-8: Consistent parent upgrade; OK to merge.

No functional changes here; ensure CI uses the same parent across all store submodules.

Confirm with the repo-wide scan script that all registry-store children are on 6.6.1-auto-regulator.

server/common/util/pom.xml (1)

8-8: Parent version updated correctly.

No concerns with the version bump itself.

server/remoting/api/pom.xml (1)

8-8: Version alignment confirmed for remoting-api.

Looks consistent with the rest of the remoting stack.

Please include this module in the parent-version sweep using the earlier script.

server/server/shared/pom.xml (1)

8-8: Shared server module now inherits 6.6.1-auto-regulator; LGTM.

No other changes detected.

server/server/data/pom.xml (1)

8-8: Parent upgraded; OK.

Note: excluding registry-store-jdbc transitively and adding it directly later in this POM is fine and intentional.

Run the repo-wide scan to ensure all server/server/* modules are on 6.6.1-auto-regulator.

server/remoting/bolt/pom.xml (1)

8-8: Parent version bump verified — no leftover 6.6.0 parents

All 28 pom.xml parent entries use 6.6.1-auto-regulator; no occurrences of 6.6.0 found.

server/server/session/src/test/java/com/alipay/sofa/registry/server/session/store/SessionInterestsTest.java (2)

285-308: LGTM: subscriber count by app path

Construction of DataInfoId and assertions align with DataInfo.toDataInfoId contract.


319-327: LGTM: createSubscriber helper

Setting group before recomputing dataInfoId is correct; matches test expectations.

server/remoting/http/pom.xml (1)

8-8: Version bump OK

Parent version update to 6.6.1-auto-regulator is consistent with the PR.

server/server/integration/pom.xml (1)

8-8: Version bump OK

Matches the repository-wide parent update.

server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java (1)

418-421: Wire-up for DataCenterResource looks correct

Bean exposure enables jersey scanning via Path for registration.

server/server/session/src/main/resources/log4j2.xml (1)

1140-1143: Logger wiring OK

Name is clear; confirm regulator code logs with “AUTO-PUSH-EFFICIENCY-REGULATOR”.

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/ChangeProcessor.java (2)

67-86: Expose snapshot of per‑DC worker debouncing config — looks good.

Iteration over ConcurrentMap is fine; returning new value objects avoids leaking mutability.


244-246: Accessor looks good.

Returns an immutable snapshot object per call; no leakage of internal state.

client/api/pom.xml (1)

8-8: Parent version aligned to 6.6.1-auto-regulator.

Looks consistent with the repo-wide bump.

server/store/pom.xml (1)

8-8: Parent version bump OK.

server/common/pom.xml (1)

8-8: Parent version bump OK.

core/pom.xml (1)

8-8: Parent version bump OK.

server/common/model/pom.xml (1)

8-8: Parent version bump OK.

server/server/session/pom.xml (1)

8-8: Parent version bump OK.

server/pom.xml (1)

10-10: Stale version found — update ./VERSION
No pom.xml contains 6.6.0; ./VERSION (line 1) contains "6.6.0". Update it to "6.6.1-auto-regulator" or confirm it's intentional.

client/pom.xml (1)

10-10: Parent version aligned.

Looks consistent with the train; keep root pom.xml in sync.

client/impl/pom.xml (1)

8-8: LGTM on parent bump.

No other changes in this module; safe.

test/pom.xml (1)

8-8: Test module parent updated.

Ensure any CI matrix that pins version strings is updated accordingly.

server/server/pom.xml (1)

8-8: Server aggregator parent updated.

Downstream submodules should inherit the same parent; please confirm no mixed parents remain.

client/log/pom.xml (1)

8-8: LGTM.

Parent version change only.

client/all/pom.xml (1)

9-9: Artifact version set to 6.6.1-auto-regulator.

All shaded deps reference ${project.version}, so this stays coherent.

server/distribution/pom.xml (1)

9-9: Distribution parent aligned.

No other diff; good to go.

server/server/meta/pom.xml (1)

8-8: Version bump to 6.6.1-auto-regulator looks good — verify all parent versions and remove any 6.6.0 remnants

Automated verification failed in the sandbox (Python heredoc parse error). Run the script below locally and paste the output.

#!/bin/bash
set -euo pipefail
TARGET='6.6.1-auto-regulator'

echo "Searching for leftover 6.6.0 in pom.xml files..."
rg -n --hidden --glob '!**/target/**' --glob '**/pom.xml' -S '6\.6\.0' || true

echo
echo "Verifying all parent versions == $TARGET..."
bad=0
while IFS= read -r p; do
  ver=$(awk 'BEGIN{RS="</parent>"} /<parent>/ { if (match($0, /<version>[[:space:]]*([^<]*)[[:space:]]*<\/version>/, a)) { print a[1]; exit } }' "$p" || true)
  [ -z "$ver" ] && continue
  [ "$ver" = "$TARGET" ] && continue
  echo "MISMATCH: $p -> $ver"
  bad=1
done < <(fd -H -a 'pom.xml')

if [ "$bad" -eq 0 ]; then
  echo "All parents aligned to $TARGET"
  exit 0
else
  exit 2
fi
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushProcessor.java (2)

70-71: CircuitBreakerService injection unchanged.

No issues spotted.


72-106: Volatile regulator + setter: thread-safety acceptable — setter is wired.
Volatile read with a local snapshot avoids repeated volatile hits; PushEfficiencyConfigUpdater calls setAutoPushEfficiencyRegulator(...) at server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java:121.

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyImproveConfig.java (2)

258-265: Accessor methods LGTM.

No issues.


84-86: New AutoPushEfficiencyConfig hook added — confirm provider JSON key

AutoPushEfficiencyConfig is declared and used in Java (server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyImproveConfig.java:85; PushEfficiencyConfigUpdater.java:76; AutoPushEfficiencyRegulator.java:89) and covered by tests, but no provider JSON entry for the key was found. Ensure the provider JSON uses the exact key autoPushEfficiencyConfig (case‑sensitive) so it binds as expected.

server/server/session/src/test/java/com/alipay/sofa/registry/server/session/providedata/FetchPushEfficiencyConfigServiceTest.java (2)

25-25: Import shift to PushEfficiencyConfigUpdater is correct.


42-43: Builder now uses a single updater dependency — cleaner API.

Test wiring is simplified and clearer.

server/remoting/pom.xml (1)

8-8: Parent alignment confirmed — no parents remain on 6.6.0

ripgrep found 0 occurrences of parent version 6.6.0 and 155 pom.xml parents reference 6.6.1-auto-regulator.

server/distribution/all/pom.xml (1)

8-8: Approve — parent aligned to 6.6.1-auto-regulator

No functional changes; distribution descriptor referenced at server/distribution/all/pom.xml:73.

pom.xml (1)

9-9: Confirm release/distribution expectations for "6.6.1-auto-regulator"

pom.xml (line ~9): 6.6.1-auto-regulator. If this is an internal build tag, ensure release pipelines do not publish it to OSSRH; if intended as a public/pre-release, document the suffix and release plan.

Quick grep for parent references to 6.6.0 returned no matches. If you expected results, re-run or manually verify parent/module pom.xml files with:

rg -nPU --glob '**/pom.xml' '(?s)<parent>.*?<artifactId>registry-.*?</artifactId>.*?<version>\s*6\.6\.0\s*</version>.*?</parent>'
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/ChangeDebouncingTime.java (1)

23-51: LGTM.

Simple POJO with no side effects; suitable for JSON serialization.

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchPushEfficiencyConfigService.java (1)

96-100: Setter LGTM for tests

Keeping the testing-only setter for SessionServerConfig is fine.

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java (2)

121-129: Propagation LGTM

Config propagation to Change/Push/Fire processors is consistent.


131-133: Confirm traffic-limit semantics; inversion may confuse

Here you unconditionally call setEnableTrafficOperate(true) (“关闭限流”), while updateTrafficOperateLimitSwitch(...) inverts the flag (setEnableTrafficOperate(!trafficOperateLimitSwitch)). Please confirm the intended semantics and whether unconditional “enable operate” on every provider update could override an admin’s limit choice.

Would you like me to scan usages of setEnableTrafficOperate and trafficOperateLimitSwitch across the repo to confirm end‑to‑end behavior?

Also applies to: 150-160

Comment on lines +86 to +87
this.pushEfficiencyConfigUpdater.updateFromProviderData(pushEfficiencyImproveConfig);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Harden against updater failures (wrap and fail fast)

If updateFromProviderData(...) throws, the exception will bubble out and may disrupt the fetch loop unpredictably. Wrap and log, and return false to signal failure.

Apply this diff:

-    this.pushEfficiencyConfigUpdater.updateFromProviderData(pushEfficiencyImproveConfig);
+    try {
+      this.pushEfficiencyConfigUpdater.updateFromProviderData(pushEfficiencyImproveConfig);
+    } catch (Throwable t) {
+      LOGGER.error("updateFromProviderData failed, value={}", pushEfficiencyImproveConfig, t);
+      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.

Suggested change
this.pushEfficiencyConfigUpdater.updateFromProviderData(pushEfficiencyImproveConfig);
try {
this.pushEfficiencyConfigUpdater.updateFromProviderData(pushEfficiencyImproveConfig);
} catch (Throwable t) {
LOGGER.error("updateFromProviderData failed, value={}", pushEfficiencyImproveConfig, t);
return false;
}

Comment on lines +186 to +188
public boolean isEnableTrafficOperateLimitSwitch() {
return enableTrafficOperateLimitSwitch;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Missing setter for enableTrafficOperateLimitSwitch prevents enabling via config.

Expose a setter to allow toggling this feature from provide‑data/config binders.

   public boolean isEnableTrafficOperateLimitSwitch() {
     return enableTrafficOperateLimitSwitch;
   }
+
+  public void setEnableTrafficOperateLimitSwitch(boolean enableTrafficOperateLimitSwitch) {
+    this.enableTrafficOperateLimitSwitch = enableTrafficOperateLimitSwitch;
+  }
📝 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.

Suggested change
public boolean isEnableTrafficOperateLimitSwitch() {
return enableTrafficOperateLimitSwitch;
}
public boolean isEnableTrafficOperateLimitSwitch() {
return enableTrafficOperateLimitSwitch;
}
public void setEnableTrafficOperateLimitSwitch(boolean enableTrafficOperateLimitSwitch) {
this.enableTrafficOperateLimitSwitch = enableTrafficOperateLimitSwitch;
}
🤖 Prompt for AI Agents
In
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyConfig.java
around lines 186-188, the boolean property enableTrafficOperateLimitSwitch has a
getter but no setter, preventing configuration binding; add a public setter
method named setEnableTrafficOperateLimitSwitch that accepts a boolean parameter
and assigns it to the enableTrafficOperateLimitSwitch field so
configuration/providers can toggle the flag.

Comment on lines +92 to +107
// 初始化窗口相关配置
this.windowTime = autoPushEfficiencyConfig.getWindowTimeMillis();
this.windowNum = autoPushEfficiencyConfig.getWindowNum();
this.windows = new AtomicLong[windowNum];
for (int i = 0; i < windowNum; i++) {
this.windows[i] = new AtomicLong(0);
}
this.index = new AtomicInteger(0);

// 设置其他参数
this.id = ID_GENERATOR.incrementAndGet();
this.pushCountThreshold = autoPushEfficiencyConfig.getPushCountThreshold();
this.loadThreshold = autoPushEfficiencyConfig.getLoadThreshold();
this.warmupTimes = 0;
this.pushEfficiencyConfigUpdater = pushEfficiencyConfigUpdater;
this.operatingSystemMXBean = ManagementFactory.getOperatingSystemMXBean();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Validate windowNum and windowTime to prevent misconfiguration and hot loops.

A zero/negative windowNum leads to AIOOBE; zero/negative windowTime can cause tight spinning or IllegalArgumentException.

     // 初始化窗口相关配置
     this.windowTime = autoPushEfficiencyConfig.getWindowTimeMillis();
     this.windowNum = autoPushEfficiencyConfig.getWindowNum();
+    if (this.windowNum <= 0) {
+      throw new IllegalArgumentException("windowNum must be > 0");
+    }
+    if (this.windowTime <= 0) {
+      throw new IllegalArgumentException("windowTimeMillis must be > 0");
+    }
📝 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.

Suggested change
// 初始化窗口相关配置
this.windowTime = autoPushEfficiencyConfig.getWindowTimeMillis();
this.windowNum = autoPushEfficiencyConfig.getWindowNum();
this.windows = new AtomicLong[windowNum];
for (int i = 0; i < windowNum; i++) {
this.windows[i] = new AtomicLong(0);
}
this.index = new AtomicInteger(0);
// 设置其他参数
this.id = ID_GENERATOR.incrementAndGet();
this.pushCountThreshold = autoPushEfficiencyConfig.getPushCountThreshold();
this.loadThreshold = autoPushEfficiencyConfig.getLoadThreshold();
this.warmupTimes = 0;
this.pushEfficiencyConfigUpdater = pushEfficiencyConfigUpdater;
this.operatingSystemMXBean = ManagementFactory.getOperatingSystemMXBean();
// 初始化窗口相关配置
this.windowTime = autoPushEfficiencyConfig.getWindowTimeMillis();
this.windowNum = autoPushEfficiencyConfig.getWindowNum();
if (this.windowNum <= 0) {
throw new IllegalArgumentException("windowNum must be > 0");
}
if (this.windowTime <= 0) {
throw new IllegalArgumentException("windowTimeMillis must be > 0");
}
this.windows = new AtomicLong[windowNum];
for (int i = 0; i < windowNum; i++) {
this.windows[i] = new AtomicLong(0);
}
this.index = new AtomicInteger(0);
// 设置其他参数
this.id = ID_GENERATOR.incrementAndGet();
this.pushCountThreshold = autoPushEfficiencyConfig.getPushCountThreshold();
this.loadThreshold = autoPushEfficiencyConfig.getLoadThreshold();
this.warmupTimes = 0;
this.pushEfficiencyConfigUpdater = pushEfficiencyConfigUpdater;
this.operatingSystemMXBean = ManagementFactory.getOperatingSystemMXBean();
🤖 Prompt for AI Agents
In
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyRegulator.java
around lines 92 to 107, validate autoPushEfficiencyConfig.getWindowNum() and
getWindowTimeMillis(): ensure windowNum > 0 and windowTime > 0 before allocating
the windows array and using them; if either is <= 0, throw an
IllegalArgumentException with a clear message (or fallback to safe default
values if project convention prefers), so you avoid
ArrayIndexOutOfBoundsException and tight spin loops; then proceed to allocate
AtomicLong[windowNum], initialize each element, and set index and other fields
only after validation.

Comment on lines +88 to +98
public void setChangeDebouncingMillis(int changeDebouncingMillis, int changeDebouncingMaxMillis) {
for (Map.Entry<String, Worker[]> entry : dataCenterWorkers.entrySet()) {
Worker[] workers = entry.getValue();
if (workers == null) {
return;
}
for (Worker work : workers) {
work.setChangeDebouncingMillis(changeDebouncingMillis, changeDebouncingMaxMillis);
}
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Early return aborts propagation to remaining data centers + missing input validation.

  • If one entry has null workers, the method returns early and skips updating other DCs.
  • No validation for negative values or max < base, which can break scheduling semantics.

Apply:

-  public void setChangeDebouncingMillis(int changeDebouncingMillis, int changeDebouncingMaxMillis) {
-    for (Map.Entry<String, Worker[]> entry : dataCenterWorkers.entrySet()) {
-      Worker[] workers = entry.getValue();
-      if (workers == null) {
-        return;
-      }
-      for (Worker work : workers) {
-        work.setChangeDebouncingMillis(changeDebouncingMillis, changeDebouncingMaxMillis);
-      }
-    }
-  }
+  public void setChangeDebouncingMillis(int changeDebouncingMillis, int changeDebouncingMaxMillis) {
+    if (changeDebouncingMillis < 0 || changeDebouncingMaxMillis < 0) {
+      LOGGER.warn("Ignore invalid debouncing values: millis={}, maxMillis={}",
+          changeDebouncingMillis, changeDebouncingMaxMillis);
+      return;
+    }
+    if (changeDebouncingMaxMillis < changeDebouncingMillis) {
+      LOGGER.warn("Clamp maxMillis {} to millis {} to keep max>=base",
+          changeDebouncingMaxMillis, changeDebouncingMillis);
+      changeDebouncingMaxMillis = changeDebouncingMillis;
+    }
+    for (Map.Entry<String, Worker[]> entry : dataCenterWorkers.entrySet()) {
+      Worker[] workers = entry.getValue();
+      if (workers == null) {
+        continue;
+      }
+      for (Worker work : workers) {
+        work.setChangeDebouncingMillis(changeDebouncingMillis, changeDebouncingMaxMillis);
+      }
+    }
+  }

Note: setWorkDelayTime(...) above has the same premature return; consider switching it to continue too for consistency.

🤖 Prompt for AI Agents
In
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/ChangeProcessor.java
around lines 88 to 98, the method currently returns early when encountering a
null Worker[] and lacks validation for the debouncing values; change the early
return to continue so other data centers are still processed, and add input
validation at the start of the method to reject negative changeDebouncingMillis
or changeDebouncingMaxMillis and to ensure changeDebouncingMaxMillis >=
changeDebouncingMillis (throw IllegalArgumentException on invalid inputs); also
apply the same return->continue fix to setWorkDelayTime(...) above for
consistency.

Comment on lines +159 to 161
volatile int changeDebouncingMillis;
volatile int changeDebouncingMaxMillis;
int changeTaskWaitingMillis = 100;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Visibility race: changeTaskWaitingMillis is not volatile.

It’s updated via setChangeTaskWorkDelay(...) and read by getWaitingMillis() on the worker thread; without volatile (or synchronized reads), updates may not be observed.

-    volatile int changeDebouncingMillis;
-    volatile int changeDebouncingMaxMillis;
-    int changeTaskWaitingMillis = 100;
+    volatile int changeDebouncingMillis;
+    volatile int changeDebouncingMaxMillis;
+    volatile int changeTaskWaitingMillis = 100;
📝 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.

Suggested change
volatile int changeDebouncingMillis;
volatile int changeDebouncingMaxMillis;
int changeTaskWaitingMillis = 100;
volatile int changeDebouncingMillis;
volatile int changeDebouncingMaxMillis;
volatile int changeTaskWaitingMillis = 100;
🤖 Prompt for AI Agents
In
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/ChangeProcessor.java
around lines 159-161, the field changeTaskWaitingMillis is updated from one
thread and read from the worker thread but is not volatile, causing a visibility
race; make the field volatile (volatile int changeTaskWaitingMillis = 100;) so
writes in setChangeTaskWorkDelay(...) are reliably observed by
getWaitingMillis(), or alternatively replace it with an AtomicInteger and update
the setter/getter to use atomic methods to ensure proper visibility.

Comment on lines +165 to +192
@Override
public void start() {}

@Override
public void stop() {
// Bean 被销毁的时候需要清理释放线程资源
this.lock.lock();
try {
if (!this.stop) {
this.stop = true;
if (null != this.autoPushEfficiencyRegulator) {
this.autoPushEfficiencyRegulator.close();
}
}
} finally {
this.lock.unlock();
}
}

@Override
public boolean isRunning() {
this.lock.lock();
try {
return this.stop;
} finally {
this.lock.unlock();
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

SmartLifecycle contract broken: isRunning() inverted; start() no-op; resource cleanup incomplete

  • isRunning() returns stop instead of !stop.
  • start() should reset stop=false.
  • On stop(), also null out the regulator and detach it from PushProcessor to prevent stale references.

Apply this diff:

   @Override
-  public void start() {}
+  public void start() {
+    this.lock.lock();
+    try {
+      this.stop = false;
+    } finally {
+      this.lock.unlock();
+    }
+  }

   @Override
   public void stop() {
     // Bean 被销毁的时候需要清理释放线程资源
     this.lock.lock();
     try {
       if (!this.stop) {
         this.stop = true;
         if (null != this.autoPushEfficiencyRegulator) {
           this.autoPushEfficiencyRegulator.close();
+          this.autoPushEfficiencyRegulator = null;
         }
+        this.useAutoPushEfficiency = false;
+        this.pushProcessor.setAutoPushEfficiencyRegulator(null);
       }
     } finally {
       this.lock.unlock();
     }
   }

   @Override
   public boolean isRunning() {
     this.lock.lock();
     try {
-      return this.stop;
+      return !this.stop;
     } finally {
       this.lock.unlock();
     }
   }

Also applies to: 169-181

🤖 Prompt for AI Agents
In
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java
around lines 165-192 (also applies to 169-181), isRunning() currently returns
stop (inverted), start() is a no-op, and stop() closes the regulator but leaves
stale references; change isRunning() to return !stop, implement start() to set
stop = false (and initialize/reset any state as needed), and in stop() after
closing autoPushEfficiencyRegulator, also set autoPushEfficiencyRegulator = null
and detach it from the PushProcessor (remove/clear any reference held by
PushProcessor) to avoid stale references. Ensure all lock usage remains correct
around these updates.

Comment on lines +50 to +55
if (!AuthChecker.authCheck(token)) {
LOGGER.error(
"[module=PushEfficiencyConfigResource][method=getChangeDebouncingMillis] auth check={} fail!",
token);
return new GenericResponse().fillFailed("auth check fail");
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Do not log authentication tokens; downgrade log level.

Leaking the full admin token in logs is a security vulnerability. Log a generic message at WARN and avoid printing the token value.

Apply this diff:

-      if (!AuthChecker.authCheck(token)) {
-        LOGGER.error(
-            "[module=PushEfficiencyConfigResource][method=getChangeDebouncingMillis] auth check={} fail!",
-            token);
-        return new GenericResponse().fillFailed("auth check fail");
-      }
+      if (!AuthChecker.authCheck(token)) {
+        LOGGER.warn(
+            "[module=PushEfficiencyConfigResource][method=getChangeDebouncingMillis] auth check failed");
+        return new GenericResponse<Map<String, ChangeDebouncingTime[]>>()
+            .fillFailed("auth check fail");
+      }
📝 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.

Suggested change
if (!AuthChecker.authCheck(token)) {
LOGGER.error(
"[module=PushEfficiencyConfigResource][method=getChangeDebouncingMillis] auth check={} fail!",
token);
return new GenericResponse().fillFailed("auth check fail");
}
if (!AuthChecker.authCheck(token)) {
LOGGER.warn(
"[module=PushEfficiencyConfigResource][method=getChangeDebouncingMillis] auth check failed");
return new GenericResponse<Map<String, ChangeDebouncingTime[]>>()
.fillFailed("auth check fail");
}
🤖 Prompt for AI Agents
In
server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/PushEfficiencyConfigResource.java
around lines 50 to 55, the code currently logs the full authentication token at
ERROR; change this to log a generic warning without the token and downgrade the
level to WARN. Replace the LOGGER.error(...) call with a LOGGER.warn(...) that
contains a generic message such as
"[module=PushEfficiencyConfigResource][method=getChangeDebouncingMillis] auth
check failed" (no token or sensitive data), and keep returning the same
GenericResponse().fillFailed("auth check fail").

Comment on lines +155 to +159
@Override
public Void answer(InvocationOnMock invocation) throws Throwable {
this.debouncingTime = invocation.getArgumentAt(0, Integer.class);
this.maxDebouncingTime = invocation.getArgumentAt(1, Integer.class);
return null;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Use modern Mockito API for argument access.

getArgumentAt(...) is deprecated; use getArgument(index, type).

-    this.debouncingTime = invocation.getArgumentAt(0, Integer.class);
-    this.maxDebouncingTime = invocation.getArgumentAt(1, Integer.class);
+    this.debouncingTime = invocation.getArgument(0, Integer.class);
+    this.maxDebouncingTime = invocation.getArgument(1, Integer.class);
📝 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.

Suggested change
@Override
public Void answer(InvocationOnMock invocation) throws Throwable {
this.debouncingTime = invocation.getArgumentAt(0, Integer.class);
this.maxDebouncingTime = invocation.getArgumentAt(1, Integer.class);
return null;
@Override
public Void answer(InvocationOnMock invocation) throws Throwable {
this.debouncingTime = invocation.getArgument(0, Integer.class);
this.maxDebouncingTime = invocation.getArgument(1, Integer.class);
return null;
}
🤖 Prompt for AI Agents
In
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyRegulatorTest.java
around lines 155 to 159, the mock Answer implementation uses the deprecated
getArgumentAt(...) method; replace calls to invocation.getArgumentAt(0,
Integer.class) and invocation.getArgumentAt(1, Integer.class) with the modern
Mockito API invocation.getArgument(0, Integer.class) and
invocation.getArgument(1, Integer.class) respectively to avoid deprecation
warnings and use the supported signature.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (6)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java (2)

30-31: Add a timeout to prevent hangs in CI

Guard against a stuck close/join by adding a small timeout to the test.

-  @Test
+  @Test(timeout = 5_000)

37-42: Simplify wiring with @InjectMocks/@mock

Use Mockito’s @Injectmocks and @mock + MockitoJUnitRunner/MockitoExtension to reduce boilerplate.

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java (4)

47-58: Make lock final; drop ctor init

Lock never changes; prefer final field initialization.

-  private Lock lock;
+  private final Lock lock = new ReentrantLock();
   private boolean stop;
@@
-  public PushEfficiencyConfigUpdater() {
-    this.lock = new ReentrantLock();
-    this.stop = false;
-  }
+  public PushEfficiencyConfigUpdater() {
+    this.stop = false;
+  }

65-71: Null-guard provider data to avoid NPE

Return early if provider data is null.

   public void updateFromProviderData(PushEfficiencyImproveConfig pushEfficiencyImproveConfig) {
     this.lock.lock();
     try {
+      if (pushEfficiencyImproveConfig == null) {
+        LOGGER.warn("[PushEfficiencyConfigUpdater] skip update: provider data is null");
+        return;
+      }
       LOGGER.info(
           "[PushEfficiencyConfigUpdater] update config from provider data: {}",
           pushEfficiencyImproveConfig);

84-91: Avoid closing regulator while holding the lock

close() may block or call back; release the lock before closing to reduce contention/deadlock risk.

Example pattern:

AutoPushEfficiencyRegulator toClose = null;
lock.lock();
try {
  toClose = this.autoPushEfficiencyRegulator;
  this.autoPushEfficiencyRegulator = null;
  // ... decide next state/new regulator
} finally {
  lock.unlock();
}
if (toClose != null) {
  toClose.close();
}

92-104: Don’t mutate inbound provider config; use a defensive copy

updateFromProviderData mutates pushEfficiencyImproveConfig (sets debouncing mins). If callers reuse the object, this leaks side effects. Copy needed fields into a new config instance and mutate the copy.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 693e798 and 8d911ec.

📒 Files selected for processing (2)
  • server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java (1 hunks)
  • server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build
  • GitHub Check: build
🔇 Additional comments (4)
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java (2)

43-49: LGTM: validates null regulator on default config

Good initial-state assertion.


79-97: LGTM: verifies new regulator and previous closed

Checks for id bump and closure of the old regulator look correct.

server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java (2)

121-133: Confirm traffic-operate semantics

Comment says “先关闭掉限流”, but the call sets setEnableTrafficOperate(true). Confirm that “true” indeed disables throttling/allows operations; otherwise invert.


166-193: SmartLifecycle contract broken: isRunning inverted; start no-op; stop leaves stale refs

Implement start(), invert isRunning(), and on stop() clear flags and detach regulator from PushProcessor.

   @Override
-  public void start() {}
+  public void start() {
+    this.lock.lock();
+    try {
+      this.stop = false;
+    } finally {
+      this.lock.unlock();
+    }
+  }
@@
   @Override
   public void stop() {
     // Bean 被销毁的时候需要清理释放线程资源
     this.lock.lock();
     try {
       if (!this.stop) {
         this.stop = true;
         if (null != this.autoPushEfficiencyRegulator) {
           this.autoPushEfficiencyRegulator.close();
+          this.autoPushEfficiencyRegulator = null;
         }
+        this.useAutoPushEfficiency = false;
+        this.pushProcessor.setAutoPushEfficiencyRegulator(null);
       }
     } finally {
       this.lock.unlock();
     }
   }
@@
   @Override
   public boolean isRunning() {
     this.lock.lock();
     try {
-      return this.stop;
+      return !this.stop;
     } finally {
       this.lock.unlock();
     }
   }

Comment on lines +98 to +99
// 清理释放线程资源
pushEfficiencyConfigUpdater.stop();

// 新的 AutoPushEfficiencyRegulator 也应当为关闭状态
Assert.assertTrue(newAutoPushEfficiencyRegulator.isClosed());
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

🧩 Analysis chain

Also assert detachment and traffic-toggle side effects

Stop should detach the regulator from PushProcessor and the updater should toggle traffic-operate each update. Add these verifications; the first will fail until the lifecycle bug in PushEfficiencyConfigUpdater is fixed.

   // 清理释放线程资源
   pushEfficiencyConfigUpdater.stop();

   // 新的 AutoPushEfficiencyRegulator 也应当为关闭状态
   Assert.assertTrue(newAutoPushEfficiencyRegulator.isClosed());
+
+  // PushProcessor 应解绑 regulator(停止后应为 null)
+  Mockito.verify(pushProcessor).setAutoPushEfficiencyRegulator(null);
+
+  // 三次更新均应关闭限流(允许操作)
+  Mockito.verify(clientManagerResource, Mockito.times(3)).setEnableTrafficOperate(true);

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Searching for test and implementation files..."

TEST_FILE="server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java"
IMPLEMENTATION="server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java"

# show test file if present
if [ -f "$TEST_FILE" ]; then
  echo "=== Found test file: $TEST_FILE (showing lines 1-240) ==="
  nl -ba "$TEST_FILE" | sed -n '1,240p'
else
  echo "Test file not found at expected path. Searching repo for file name..."
  rg -n --hidden --follow "PushEfficiencyConfigUpdaterTest" -S || true
fi

# show implementation file if present
if [ -f "$IMPLEMENTATION" ]; then
  echo "=== Found implementation: $IMPLEMENTATION (showing lines 1-320) ==="
  nl -ba "$IMPLEMENTATION" | sed -n '1,320p'
else
  echo "Implementation not found at expected path. Searching repo for class name..."
  rg -n --hidden --follow "class PushEfficiencyConfigUpdater" -S || true
fi

# search for related symbols and mock/verifications in tests
echo "=== Searching for related symbols: setAutoPushEfficiencyRegulator, setEnableTrafficOperate, clientManagerResource, pushProcessor, AutoPushEfficiencyRegulator ==="
rg -n --hidden --follow "setAutoPushEfficiencyRegulator" -S || true
rg -n --hidden --follow "setEnableTrafficOperate" -S || true
rg -n --hidden --follow "clientManagerResource" -S || true
rg -n --hidden --follow "pushProcessor" -S || true
rg -n --hidden --follow "AutoPushEfficiencyRegulator" -S || true

echo "=== Searching tests for Mockito.verify usages ==="
rg -n --hidden --follow "Mockito.verify" server -S || true

Length of output: 253


🏁 Script executed:

#!/bin/bash
set -euo pipefail

TEST_FILE="server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java"
IMPL_FILE="server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java"

echo "---- Show test file (1-240) ----"
if [ -f "$TEST_FILE" ]; then
  sed -n '1,240p' "$TEST_FILE" || true
else
  echo "Test file not found at $TEST_FILE"
  rg -n "PushEfficiencyConfigUpdaterTest" -S || true
fi

echo "---- Show test file (240-520) ----"
if [ -f "$TEST_FILE" ]; then
  sed -n '240,520p' "$TEST_FILE" || true
fi

echo "---- Show implementation (1-400) ----"
if [ -f "$IMPL_FILE" ]; then
  sed -n '1,400p' "$IMPL_FILE" || true
else
  echo "Implementation not found at $IMPL_FILE"
  rg -n "class PushEfficiencyConfigUpdater" -S || true
fi

echo "---- Search occurrences: setAutoPushEfficiencyRegulator ----"
rg -n "setAutoPushEfficiencyRegulator" -S || true

echo "---- Search occurrences: setEnableTrafficOperate ----"
rg -n "setEnableTrafficOperate" -S || true

echo "---- Search occurrences: clientManagerResource ----"
rg -n "clientManagerResource" -S || true

echo "---- Locate PushProcessor class ----"
pp_file=$(rg -n "class PushProcessor" -S | head -n1 | cut -d: -f1 || true)
if [ -n "$pp_file" ]; then
  echo "Found PushProcessor in: $pp_file"
  sed -n '1,320p' "$pp_file" || true
else
  rg -n "PushProcessor" -S || true
fi

echo "---- Locate AutoPushEfficiencyRegulator class ----"
apr_file=$(rg -n "class AutoPushEfficiencyRegulator" -S | head -n1 | cut -d: -f1 || true)
if [ -n "$apr_file" ]; then
  echo "Found AutoPushEfficiencyRegulator in: $apr_file"
  sed -n '1,320p' "$apr_file" || true
else
  rg -n "AutoPushEfficiencyRegulator" -S || true
fi

echo "---- Searching tests for Mockito.verify usage ----"
rg -n "Mockito.verify" server -S || true

Length of output: 53672


Assert detachment and traffic-toggle side effects

Add two verifications; the first will fail because PushEfficiencyConfigUpdater.stop() currently closes the regulator but does not detach it from PushProcessor — fix PushEfficiencyConfigUpdater.stop() to call pushProcessor.setAutoPushEfficiencyRegulator(null).

   // 清理释放线程资源
   pushEfficiencyConfigUpdater.stop();

   // 新的 AutoPushEfficiencyRegulator 也应当为关闭状态
   Assert.assertTrue(newAutoPushEfficiencyRegulator.isClosed());
+
+  // PushProcessor 应解绑 regulator(停止后应为 null)
+  Mockito.verify(pushProcessor).setAutoPushEfficiencyRegulator(null);
+
+  // 三次更新均应关闭限流(允许操作)
+  Mockito.verify(clientManagerResource, Mockito.times(3)).setEnableTrafficOperate(true);
🤖 Prompt for AI Agents
In
server/server/session/src/test/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdaterTest.java
around lines 98-103, the test expects PushEfficiencyConfigUpdater.stop() to both
close the regulator and detach it from PushProcessor; currently stop() only
closes it. Update PushEfficiencyConfigUpdater.stop() to, after safely closing
the AutoPushEfficiencyRegulator (with any necessary
null-checks/synchronization), call
pushProcessor.setAutoPushEfficiencyRegulator(null) to detach it so the test
assertions about detachment and traffic-toggle side effects pass.

@hui-cha hui-cha force-pushed the feature/do-push-optimization branch from 680d416 to 5a73faa Compare September 22, 2025 08:33
@NickNYU
Copy link
Contributor

NickNYU commented Sep 23, 2025

@coderabbitai generate docstrings

@coderabbitai
Copy link

coderabbitai bot commented Sep 23, 2025

✅ Actions performed

Docstrings generation started.

coderabbitai bot added a commit that referenced this pull request Sep 23, 2025
Docstrings generation was requested by @NickNYU.

* #372 (comment)

The following files were modified:

* `server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerBootstrap.java`
* `server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/bootstrap/MetaServerConfiguration.java`
* `server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/DataCenterResource.java`
* `server/server/meta/src/main/java/com/alipay/sofa/registry/server/meta/resource/MultiClusterSyncResource.java`
* `server/server/session/src/main/java/com/alipay/sofa/registry/server/session/bootstrap/SessionServerConfiguration.java`
* `server/server/session/src/main/java/com/alipay/sofa/registry/server/session/providedata/FetchPushEfficiencyConfigService.java`
* `server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyConfig.java`
* `server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/AutoPushEfficiencyRegulator.java`
* `server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/ChangeDebouncingTime.java`
* `server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/ChangeProcessor.java`
* `server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyConfigUpdater.java`
* `server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushEfficiencyImproveConfig.java`
* `server/server/session/src/main/java/com/alipay/sofa/registry/server/session/push/PushProcessor.java`
* `server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/ClientManagerResource.java`
* `server/server/session/src/main/java/com/alipay/sofa/registry/server/session/resource/PushEfficiencyConfigResource.java`
@coderabbitai
Copy link

coderabbitai bot commented Sep 23, 2025

Note

Generated docstrings for this pull request at #373

}

protected Collection<AbstractServerHandler> customSessionServerHandlers() {
return Collections.emptyList();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use new ArrayList<>() or Lists.newArrayList() somehow, as Collections.emptyList() is immutable
When customSessionServerHandlers is used somewhere else, it throws an error

    /**
     * The empty list (immutable).  This list is serializable.
     *
     * @see #emptyList()
     */
    @SuppressWarnings("rawtypes")
    public static final List EMPTY_LIST = new EmptyList<>();

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK,我再重新提个 PR 修改一下

@huanglongchao huanglongchao merged commit 96f501b into sofastack:master Sep 23, 2025
5 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants