Peek code review

[fxchen]

Summary

Describe the goal of this PR. Mention any related Issue numbers.

Requirements

• I've read and understood the Contributing Guidelines and have done my best effort to follow them.
• I've read and agree to the Code of Conduct.

[github-actions]

Review:

1. In ClusterHpaMetricService, the publishCacheHpaMetrics method is doing too many things. It calculates the HPA scaling metrics, publishes the metrics to ZooKeeper, and acquires or refreshes the lock for scale-down operations. It would be better to split this method into smaller, more focused methods.

2. The publishCacheHpaMetrics method can be split into two methods: calculateCacheHpaMetrics and publishCacheHpaMetrics. The calculateCacheHpaMetrics method should calculate the HPA scaling metrics based on the replica and cache slot metadata. The publishCacheHpaMetrics method should publish the calculated metrics to ZooKeeper.

3. The tryCacheReplicasetLock method can be split into two methods: acquireCacheReplicasetLock and refreshCacheReplicasetLock. The acquireCacheReplicasetLock method should acquire the lock for a given replicaset, and the refreshCacheReplicasetLock method should refresh the lock for a given replicaset.

4. The persistCacheConfig method can be split into two methods: updateCacheConfig and createCacheConfig. The updateCacheConfig method should update an existing HPA metric in the metadata store, and the createCacheConfig method should create a new HPA metric in the metadata store.

5. The ClusterHpaMetricService class should be refactored to follow the Single Responsibility Principle (SRP). Each method should have a single responsibility and should be focused on doing one thing well.

Here's an example of how the code can be refactored:

public class ClusterHpaMetricService extends AbstractScheduledService {
  // ...

  @Override
  protected void runOneIteration() {
    LOG.info("Running ClusterHpaMetricService");
    try {
      Map<String, Double> cacheHpaMetrics = calculateCacheHpaMetrics();
      publishCacheHpaMetrics(cacheHpaMetrics);
    } catch (Exception e) {
      LOG.error("Error running ClusterHpaMetricService", e);
    }
  }

  private Map<String, Double> calculateCacheHpaMetrics() {
    // Calculate HPA scaling metrics based on replica and cache slot metadata
    // ...
    return cacheHpaMetrics;
  }

  private void publishCacheHpaMetrics(Map<String, Double> cacheHpaMetrics) {
    // Publish the calculated metrics to ZooKeeper
    // ...
  }

  protected boolean acquireCacheReplicasetLock(String replicaset) {
    // Acquire the lock for the given replicaset
    // ...
    return true;
  }

  protected void refreshCacheReplicasetLock(String replicaset) {
    // Refresh the lock for the given replicaset
    // ...
  }

  private void updateCacheConfig(String replicaSet, Double demandFactor) {
    // Update an existing HPA metric in the metadata store
    // ...
  }

  private void createCacheConfig(String replicaSet, Double demandFactor) {
    // Create a new HPA metric in the metadata store
    // ...
  }
}

By refactoring the code in this way, each method has a clear responsibility and is focused on doing one thing well. This improves the readability, maintainability, and testability of the code.

[fxchen]

I opened this in preparation for a talk tomorrow!