Copilot commented on code in PR #61922:
URL: https://github.com/apache/doris/pull/61922#discussion_r3014606868
##########
fe/fe-core/src/main/java/org/apache/doris/persist/gson/GsonUtils.java:
##########
@@ -931,7 +931,13 @@ public void write(JsonWriter out, T value) throws
IOException {
if (value instanceof GsonPreProcessable) {
((GsonPreProcessable) value).gsonPreProcess();
}
- delegate.write(out, value);
+ try {
+ delegate.write(out, value);
+ } finally {
+ if (value instanceof GsonPreProcessable) {
+ ((GsonPreProcessable) value).gsonPostSerialize();
+ }
+ }
Review Comment:
`gsonPostSerialize()` is guaranteed to run if `delegate.write()` throws, but
it will *not* run if `gsonPreProcess()` throws because the pre-process call is
outside the `try/finally`. That can leave transient state (set by
`gsonPreProcess()`) attached to the object after a failed serialization.
Consider moving the `gsonPreProcess()` invocation inside the `try` and guarding
the cleanup with a flag so `gsonPostSerialize()` runs whenever pre-processing
started.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudTabletInvertedIndex.java:
##########
@@ -128,5 +149,94 @@ public List<Replica> getReplicasByTabletId(long tabletId) {
@Override
protected void innerClear() {
replicaMetaMap.clear();
+ clusterPrimaryBeMap.clear();
+ clusterSecondaryMap.clear();
+ }
+
+ // ---- Central cluster-to-BE mapping accessors ----
+
+ private ConcurrentLong2LongHashMap getOrCreateClusterMap(
+ ConcurrentHashMap<String, ConcurrentLong2LongHashMap> outer,
String clusterId) {
+ return outer.computeIfAbsent(clusterId, k -> new
ConcurrentLong2LongHashMap());
+ }
+
+ // -- Primary BE --
+
+ public long getPrimaryBeId(String clusterId, long replicaId) {
+ ConcurrentLong2LongHashMap inner = clusterPrimaryBeMap.get(clusterId);
+ if (inner == null) {
+ return -1L;
+ }
+ return inner.getOrDefault(replicaId, -1L);
+ }
+
+ public void setPrimaryBeId(String clusterId, long replicaId, long beId) {
+ getOrCreateClusterMap(clusterPrimaryBeMap, clusterId).put(replicaId,
beId);
+ }
+
+ public void removePrimaryBeId(String clusterId, long replicaId) {
+ ConcurrentLong2LongHashMap inner = clusterPrimaryBeMap.get(clusterId);
+ if (inner != null) {
+ inner.remove(replicaId);
+ }
+ }
+
+ public void clearPrimaryBeForReplica(long replicaId) {
+ for (ConcurrentLong2LongHashMap inner : clusterPrimaryBeMap.values()) {
+ inner.remove(replicaId);
+ }
+ }
+
+ public Map<String, Long> getAllPrimaryClusterBeIds(long replicaId) {
+ Map<String, Long> result = new HashMap<>();
+ for (Map.Entry<String, ConcurrentLong2LongHashMap> entry :
clusterPrimaryBeMap.entrySet()) {
+ long beId = entry.getValue().getOrDefault(replicaId, -1L);
+ if (beId != -1L) {
+ result.put(entry.getKey(), beId);
+ }
+ }
+ return result;
+ }
+
+ // -- Secondary BE (beId + timestamp stored atomically as long[2]) --
+
+ private ConcurrentLong2ObjectHashMap<long[]>
getOrCreateSecondaryMap(String clusterId) {
+ return clusterSecondaryMap.computeIfAbsent(clusterId, k -> new
ConcurrentLong2ObjectHashMap<>());
+ }
+
+ /** Returns [beId, timestamp] or null if no secondary BE is set. */
+ public long[] getSecondaryBe(String clusterId, long replicaId) {
+ ConcurrentLong2ObjectHashMap<long[]> inner =
clusterSecondaryMap.get(clusterId);
+ if (inner == null) {
+ return null;
+ }
+ return inner.get(replicaId);
Review Comment:
`getSecondaryBe()` returns the internal `long[]` stored in the central map.
Since arrays are mutable, any caller could accidentally modify the returned
array and corrupt the index state. To avoid exposing mutable internal state,
consider making this method non-public and using the existing
`getSecondaryBeId()` / `getSecondaryTimestamp()` accessors, or returning an
immutable value (or a defensive copy) for the pair.
```suggestion
long[] value = inner.get(replicaId);
if (value == null) {
return null;
}
// Return a defensive copy to avoid exposing internal mutable state.
return new long[] { value[0], value[1] };
```
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/catalog/CloudReplica.java:
##########
@@ -610,37 +618,38 @@ public void updateClusterToSecondaryBe(String cluster,
long beId) {
LOG.debug("add to secondary clusterId {}, beId {}, changeTimestamp
{}, replica info {}",
cluster, beId, changeTimestamp, this);
}
- secondaryClusterToBackends.put(cluster, Pair.of(beId,
changeTimestamp));
+ getCloudInvertedIndex().setSecondaryBe(cluster, getId(), beId,
changeTimestamp);
}
public void clearClusterToBe(String cluster) {
- primaryClusterToBackend.remove(cluster);
- secondaryClusterToBackends.remove(cluster);
+ CloudTabletInvertedIndex idx = getCloudInvertedIndex();
+ idx.removePrimaryBeId(cluster, getId());
+ idx.removeSecondaryBe(cluster, getId());
}
// ATTN: This func is only used by redundant tablet report clean in bes.
// Only the master node will do the diff logic,
// so just only need to clean up secondaryClusterToBackends on the master
node.
public void checkAndClearSecondaryClusterToBe(String clusterId, long
expireTimestamp) {
- Pair<Long, Long> secondBeAndChangeTimestamp =
secondaryClusterToBackends.get(clusterId);
- if (secondBeAndChangeTimestamp == null) {
+ CloudTabletInvertedIndex idx = getCloudInvertedIndex();
+ long[] pair = idx.getSecondaryBe(clusterId, getId());
+ if (pair == null) {
return;
}
- long beId = secondBeAndChangeTimestamp.key();
- long changeTimestamp = secondBeAndChangeTimestamp.value();
+ long beId = pair[0];
+ long changeTimestamp = pair[1];
if (changeTimestamp < expireTimestamp) {
LOG.debug("remove clusterId {} secondary beId {} changeTimestamp
{} expireTimestamp {} replica info {}",
clusterId, beId, changeTimestamp, expireTimestamp, this);
- secondaryClusterToBackends.remove(clusterId);
- return;
+ idx.removeSecondaryBe(clusterId, getId());
}
}
public List<Backend> getAllPrimaryBes() {
List<Backend> result = new ArrayList<Backend>();
- primaryClusterToBackend.forEach((clusterId, beId) -> {
- if (beId != -1) {
+
getCloudInvertedIndex().getAllPrimaryClusterBeIds(getId()).forEach((clusterId,
beId) -> {
+ if (beId != -1L) {
Backend backend = Env.getCurrentSystemInfo().getBackend(beId);
result.add(backend);
}
Review Comment:
`getAllPrimaryClusterBeIds()` already filters out entries where `beId ==
-1L`, so the `if (beId != -1L)` guard inside the `forEach` is redundant now.
Removing it would simplify the logic and avoid implying `-1L` is still a
possible value here.
```suggestion
Backend backend = Env.getCurrentSystemInfo().getBackend(beId);
result.add(backend);
```
--
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]
