morrySnow commented on code in PR #44463:
URL: https://github.com/apache/doris/pull/44463#discussion_r1857667410
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java:
##########
@@ -63,18 +66,22 @@
import java.io.IOException;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.OptionalLong;
import java.util.Set;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.stream.Collectors;
-public class PaimonExternalTable extends ExternalTable implements
MTMVRelatedTableIf, MTMVBaseTableIf {
+public class PaimonExternalTable extends ExternalTable implements
MTMVRelatedTableIf, MTMVBaseTableIf, MvccTable {
private static final Logger LOG =
LogManager.getLogger(PaimonExternalTable.class);
+ private final ReentrantReadWriteLock snapshotRwLock = new
ReentrantReadWriteLock(true);
Review Comment:
i think we should use unfair lock here
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSnapshotCache.java:
##########
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.datasource.paimon;
+
+import org.apache.doris.datasource.CacheException;
+import org.apache.doris.mtmv.BaseTableInfo;
+
+import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Maps;
+
+import java.util.Map;
+import java.util.OptionalLong;
+import java.util.concurrent.atomic.AtomicInteger;
+
+public class PaimonSnapshotCache {
+ // snapshotId ==> SchemaCacheValue
+ private static Map<SnapshotCacheKey, PaimonSchemaCacheValue> snapshotCache
= Maps.newConcurrentMap();
+ // snapshotId ==> refNum
+ private static Map<SnapshotCacheKey, AtomicInteger> snapshotIdRefs =
Maps.newConcurrentMap();
+
+ public static PaimonSchemaCacheValue
getSchemaCacheBySnapshotId(PaimonExternalTable paimonExternalTable,
+ long snapshotId) throws CacheException {
+ Preconditions.checkNotNull(paimonExternalTable);
+ BaseTableInfo baseTableInfo = new BaseTableInfo(paimonExternalTable);
+ SnapshotCacheKey key = new SnapshotCacheKey(baseTableInfo, snapshotId);
+ try {
+ paimonExternalTable.readSnapshotLock();
+ if (snapshotCache.containsKey(key)) {
+ return snapshotCache.get(key);
+ }
+ } finally {
+ paimonExternalTable.readSnapshotUnlock();
+ }
+ return load(paimonExternalTable, snapshotId, key);
+ }
+
+ private static PaimonSchemaCacheValue load(PaimonExternalTable
paimonExternalTable, long snapshotId,
+ SnapshotCacheKey key) {
+ // may be slow, not in lock
+ PaimonSchemaCacheValue latestSchemaCache =
paimonExternalTable.getSchemaCache(OptionalLong.empty());
+ try {
+ paimonExternalTable.writeSnapshotLock();
Review Comment:
lock should out of try block
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonExternalTable.java:
##########
@@ -63,18 +66,22 @@
import java.io.IOException;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.OptionalLong;
import java.util.Set;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import java.util.stream.Collectors;
-public class PaimonExternalTable extends ExternalTable implements
MTMVRelatedTableIf, MTMVBaseTableIf {
+public class PaimonExternalTable extends ExternalTable implements
MTMVRelatedTableIf, MTMVBaseTableIf, MvccTable {
private static final Logger LOG =
LogManager.getLogger(PaimonExternalTable.class);
+ private final ReentrantReadWriteLock snapshotRwLock = new
ReentrantReadWriteLock(true);
Review Comment:
could we avoid use lock at all? fair write read lock will lead to dead lock
finally
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/StatementContext.java:
##########
@@ -508,6 +515,67 @@ public void addPlannerHook(PlannerHook plannerHook) {
this.plannerHooks.add(plannerHook);
}
+ /**
+ * refTables
+ *
+ * @param tables tables
+ */
+ public void refTables(Map<List<String>, TableIf> tables) {
+ if (tables == null) {
+ return;
+ }
+ for (TableIf tableIf : tables.values()) {
+ if (tableIf instanceof MvccTable) {
+ MvccTable mvccTable = (MvccTable) tableIf;
+ long snapshotId = fetchSnapshotIdIfNotExist(mvccTable);
+ mvccTable.ref(snapshotId);
+ }
+ }
+ }
+
+ /**
+ * unrefTables
+ */
+ public void unrefTables() {
+ for (Entry<BaseTableInfo, Long> entry : refSnapshotIds.entrySet()) {
Review Comment:
what will happen if we ref snapshot A here and table has been updated to
snapshot B before load cache?
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/PaimonSnapshotCache.java:
##########
@@ -0,0 +1,139 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.doris.datasource.paimon;
+
+import org.apache.doris.datasource.CacheException;
+import org.apache.doris.mtmv.BaseTableInfo;
+
+import com.google.common.base.Objects;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Maps;
+
+import java.util.Map;
+import java.util.OptionalLong;
+import java.util.concurrent.atomic.AtomicInteger;
+
+public class PaimonSnapshotCache {
+ // snapshotId ==> SchemaCacheValue
+ private static Map<SnapshotCacheKey, PaimonSchemaCacheValue> snapshotCache
= Maps.newConcurrentMap();
+ // snapshotId ==> refNum
+ private static Map<SnapshotCacheKey, AtomicInteger> snapshotIdRefs =
Maps.newConcurrentMap();
+
+ public static PaimonSchemaCacheValue
getSchemaCacheBySnapshotId(PaimonExternalTable paimonExternalTable,
+ long snapshotId) throws CacheException {
+ Preconditions.checkNotNull(paimonExternalTable);
+ BaseTableInfo baseTableInfo = new BaseTableInfo(paimonExternalTable);
+ SnapshotCacheKey key = new SnapshotCacheKey(baseTableInfo, snapshotId);
+ try {
+ paimonExternalTable.readSnapshotLock();
+ if (snapshotCache.containsKey(key)) {
+ return snapshotCache.get(key);
+ }
+ } finally {
+ paimonExternalTable.readSnapshotUnlock();
+ }
+ return load(paimonExternalTable, snapshotId, key);
+ }
+
+ private static PaimonSchemaCacheValue load(PaimonExternalTable
paimonExternalTable, long snapshotId,
+ SnapshotCacheKey key) {
+ // may be slow, not in lock
+ PaimonSchemaCacheValue latestSchemaCache =
paimonExternalTable.getSchemaCache(OptionalLong.empty());
+ try {
+ paimonExternalTable.writeSnapshotLock();
+ if (snapshotCache.containsKey(key)) {
+ return snapshotCache.get(key);
+ }
+ if (latestSchemaCache.getSnapshootId() == snapshotId) {
+ snapshotCache.put(key, latestSchemaCache);
+ return latestSchemaCache;
+ }
+ } finally {
+ paimonExternalTable.writeSnapshotUnlock();
+ }
+ throw new CacheException("schema can not find by: " + key);
+ }
+
+ public static void ref(PaimonExternalTable paimonExternalTable, long
snapshotId) {
+ Preconditions.checkNotNull(paimonExternalTable);
+ BaseTableInfo baseTableInfo = new BaseTableInfo(paimonExternalTable);
+ SnapshotCacheKey key = new SnapshotCacheKey(baseTableInfo, snapshotId);
+ try {
+ paimonExternalTable.writeSnapshotLock();
+ if (snapshotIdRefs.containsKey(key)) {
+ snapshotIdRefs.get(key).getAndIncrement();
+ } else {
+ snapshotIdRefs.put(key, new AtomicInteger(1));
+ }
Review Comment:
could we use computeIfAbsent to avoid use lock here?
--
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: commits-unsubscr...@doris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@doris.apache.org
For additional commands, e-mail: commits-h...@doris.apache.org
