Re: [PR] Supported "need last cache" semantic for table/database schema [iotdb]
jt2594838 merged PR #15538: URL: https://github.com/apache/iotdb/pull/15538 -- 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]
Re: [PR] Supported "need last cache" semantic for table/database schema [iotdb]
Caideyipi commented on code in PR #15538:
URL: https://github.com/apache/iotdb/pull/15538#discussion_r3794257801
##
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/schema/ClusterSchemaInfo.java:
##
@@ -261,6 +262,14 @@ public TSStatus alterDatabase(final DatabaseSchemaPlan
plan) {
currentSchema.getTTL());
}
+ if (alterSchema.isSetNeedLastCache()) {
+currentSchema.setNeedLastCache(alterSchema.isNeedLastCache());
+LOGGER.info(
+"[SetNeedLastCache] The need last cache flag of Database: {} is
adjusted to: {}",
Review Comment:
Applied in 280eb89dec3. The new log template now uses a `ConfigNodeMessages`
constant with matching English and Chinese definitions. Both locale compiles
pass.
##
integration-test/src/test/java/org/apache/iotdb/relational/it/schema/IoTDBTableIT.java:
##
@@ -775,10 +890,10 @@ public void testTreeViewTable() throws Exception {
try (final Connection connection = EnvFactory.getEnv().getConnection();
final Statement statement = connection.createStatement()) {
statement.execute("create database root.another");
- statement.execute("create database root.`重庆`.`1`.b");
- statement.execute("create timeSeries root.`重庆`.`1`.b.`2`.S1 int32");
- statement.execute("create timeSeries root.`重庆`.`1`.b.`2`.s2 string");
- statement.execute("create timeSeries root.`重庆`.`1`.b.S1 int32");
+ statement.execute("create database root.`閲嶅簡`.`1`.b");
+ statement.execute("create timeSeries root.`閲嶅簡`.`1`.b.`2`.S1 int32");
+ statement.execute("create timeSeries root.`閲嶅簡`.`1`.b.`2`.s2 string");
+ statement.execute("create timeSeries root.`閲嶅簡`.`1`.b.S1 int32");
Review Comment:
Applied in 280eb89dec3. This was mojibake from an incorrectly encoded `重庆`
test identifier. I restored the intended UTF-8 text in this statement and all
related tree-view paths/assertions in `IoTDBTableIT`.
##
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/analyze/cache/partition/PartitionCache.java:
##
@@ -558,12 +586,33 @@ public void checkAndAutoCreateDatabase(
/**
* update database cache
*
- * @param databaseNames the database names that need to update
+ * @param databases the database names
+ */
+ public void updateDatabaseCache(final Set databases) {
+databaseCacheLock.writeLock().lock();
+try {
+ databases.forEach(database -> database2NeedLastCacheCache.put(database,
true));
+} finally {
+ databaseCacheLock.writeLock().unlock();
+}
+ }
Review Comment:
Clarified in 280eb89dec3. This `Set` overload is only used for
databases just created by local `setDatabase` requests; those requests leave
`needLastCache` unset, whose backward-compatible default is enabled. The
Javadoc and inline comment now state that contract explicitly. The `Map` overload still preserves each fetched database's actual flag.
--
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]
Re: [PR] Supported "need last cache" semantic for table/database schema [iotdb]
jt2594838 commented on code in PR #15538:
URL: https://github.com/apache/iotdb/pull/15538#discussion_r3726907809
##
integration-test/src/test/java/org/apache/iotdb/relational/it/schema/IoTDBTableIT.java:
##
@@ -775,10 +890,10 @@ public void testTreeViewTable() throws Exception {
try (final Connection connection = EnvFactory.getEnv().getConnection();
final Statement statement = connection.createStatement()) {
statement.execute("create database root.another");
- statement.execute("create database root.`重庆`.`1`.b");
- statement.execute("create timeSeries root.`重庆`.`1`.b.`2`.S1 int32");
- statement.execute("create timeSeries root.`重庆`.`1`.b.`2`.s2 string");
- statement.execute("create timeSeries root.`重庆`.`1`.b.S1 int32");
+ statement.execute("create database root.`閲嶅簡`.`1`.b");
+ statement.execute("create timeSeries root.`閲嶅簡`.`1`.b.`2`.S1 int32");
+ statement.execute("create timeSeries root.`閲嶅簡`.`1`.b.`2`.s2 string");
+ statement.execute("create timeSeries root.`閲嶅簡`.`1`.b.S1 int32");
Review Comment:
What is this?
##
iotdb-core/confignode/src/main/java/org/apache/iotdb/confignode/persistence/schema/ClusterSchemaInfo.java:
##
@@ -261,6 +262,14 @@ public TSStatus alterDatabase(final DatabaseSchemaPlan
plan) {
currentSchema.getTTL());
}
+ if (alterSchema.isSetNeedLastCache()) {
+currentSchema.setNeedLastCache(alterSchema.isNeedLastCache());
+LOGGER.info(
+"[SetNeedLastCache] The need last cache flag of Database: {} is
adjusted to: {}",
Review Comment:
i18n
##
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/analyze/cache/partition/PartitionCache.java:
##
@@ -558,12 +586,33 @@ public void checkAndAutoCreateDatabase(
/**
* update database cache
*
- * @param databaseNames the database names that need to update
+ * @param databases the database names
+ */
+ public void updateDatabaseCache(final Set databases) {
+databaseCacheLock.writeLock().lock();
+try {
+ databases.forEach(database -> database2NeedLastCacheCache.put(database,
true));
+} finally {
+ databaseCacheLock.writeLock().unlock();
+}
+ }
Review Comment:
Why are all databases set to true in this method?
--
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]
Re: [PR] Supported "need last cache" semantic for table/database schema [iotdb]
Caideyipi commented on code in PR #15538:
URL: https://github.com/apache/iotdb/pull/15538#discussion_r3689706850
##
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/analyze/cache/partition/PartitionCache.java:
##
@@ -253,19 +253,38 @@ private void fetchDatabaseAndUpdateCache(
}
}
+ public boolean isNeedLastCache(final String database) {
+Boolean needLastCache = database2NeedLastCacheCache.get(database);
+if (Objects.nonNull(needLastCache)) {
+ return needLastCache;
+}
+try {
+ fetchDatabaseAndUpdateCache(false);
+} catch (final TException | ClientManagerException e) {
+ logger.warn(
+ "Failed to get need_last_cache info for database {}, will put cache
anyway, exception: {}",
+ database,
+ e.getMessage());
+ return true;
+}
+needLastCache = database2NeedLastCacheCache.get(database);
+return Objects.isNull(needLastCache) || needLastCache;
+ }
Review Comment:
Applied in 8a763b40815: `isNeedLastCache` is now annotated with
`@TreeModel`, and `checkAndAutoCreateDatabase` with `@TableModel`. The private
fetch helper serves both entry points, so the model annotations are kept on the
public model-specific methods. Thanks.
--
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]
Re: [PR] Supported "need last cache" semantic for table/database schema [iotdb]
Caideyipi commented on code in PR #15538:
URL: https://github.com/apache/iotdb/pull/15538#discussion_r2544128350
##
integration-test/src/test/java/org/apache/iotdb/relational/it/schema/IoTDBDatabaseIT.java:
##
@@ -617,39 +621,39 @@ public void testInformationSchema() throws SQLException {
statement.execute(
"create table test.test (a tag, b attribute, c int32 comment
'turbine') comment 'test'");
statement.execute(
- "CREATE VIEW test.view_table (tag1 STRING TAG,tag2 STRING TAG,s11
INT32 FIELD,s3 INT32 FIELD FROM s2) RESTRICT WITH (ttl=100) AS root.\"a\".**");
+ "CREATE VIEW test.view_table (tag1 STRING TAG,tag2 STRING TAG,s11
INT32 FIELD,s3 INT32 FIELD FROM s2) RESTRICT WITH (ttl=100,
need_last_cache=true) AS root.\"a\".**");
Review Comment:
The tree view will push cache in table queries, which is not restricted by
tree database configurations.
--
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]
Re: [PR] Supported "need last cache" semantic for table/database schema [iotdb]
jt2594838 commented on code in PR #15538:
URL: https://github.com/apache/iotdb/pull/15538#discussion_r2541161288
##
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/analyze/cache/partition/PartitionCache.java:
##
@@ -253,19 +253,38 @@ private void fetchDatabaseAndUpdateCache(
}
}
+ public boolean isNeedLastCache(final String database) {
+Boolean needLastCache = database2NeedLastCacheCache.get(database);
+if (Objects.nonNull(needLastCache)) {
+ return needLastCache;
+}
+try {
+ fetchDatabaseAndUpdateCache(false);
+} catch (final TException | ClientManagerException e) {
+ logger.warn(
+ "Failed to get need_last_cache info for database {}, will put cache
anyway, exception: {}",
+ database,
+ e.getMessage());
+ return true;
+}
+needLastCache = database2NeedLastCacheCache.get(database);
+return Objects.isNull(needLastCache) || needLastCache;
+ }
Review Comment:
It weird to pass a `false` to `fetchDatabaseAndUpdateCache`, because there
is no sign that indicates the method is only used by the tree model.
May add an annotation @TreeModel for this method.
The same for `checkAndAutoCreateDatabase(final String database, final
boolean isAutoCreate, final String userName)`.
##
integration-test/src/test/java/org/apache/iotdb/relational/it/schema/IoTDBDatabaseIT.java:
##
@@ -617,39 +621,39 @@ public void testInformationSchema() throws SQLException {
statement.execute(
"create table test.test (a tag, b attribute, c int32 comment
'turbine') comment 'test'");
statement.execute(
- "CREATE VIEW test.view_table (tag1 STRING TAG,tag2 STRING TAG,s11
INT32 FIELD,s3 INT32 FIELD FROM s2) RESTRICT WITH (ttl=100) AS root.\"a\".**");
+ "CREATE VIEW test.view_table (tag1 STRING TAG,tag2 STRING TAG,s11
INT32 FIELD,s3 INT32 FIELD FROM s2) RESTRICT WITH (ttl=100,
need_last_cache=true) AS root.\"a\".**");
Review Comment:
How does this apply to a tree view.
--
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]
