You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@iotdb.apache.org by GitBox <gi...@apache.org> on 2022/06/25 14:39:18 UTC
[GitHub] [iotdb] yyt86 opened a new pull request, #6442: [IOTDB-2525] Modify last cache value mechanism
yyt86 opened a new pull request, #6442:
URL: https://github.com/apache/iotdb/pull/6442
**### [Issue](https://issues.apache.org/jira/browse/IOTDB-2525)**
Currently, if a time series has no data written before, the last cache will be null, which we still need to read the disk.
**### Solution**
Add a EmptyLastCacheContainer class to indicate that this timeseries has no data.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] HTHou closed pull request #6442: [To rel/0.13][IOTDB-2525] Modify last cache value mechanism
Posted by GitBox <gi...@apache.org>.
HTHou closed pull request #6442: [To rel/0.13][IOTDB-2525] Modify last cache value mechanism
URL: https://github.com/apache/iotdb/pull/6442
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] JackieTien97 commented on a diff in pull request #6442: [To rel/0.13][IOTDB-2525] Modify last cache value mechanism
Posted by GitBox <gi...@apache.org>.
JackieTien97 commented on code in PR #6442:
URL: https://github.com/apache/iotdb/pull/6442#discussion_r915492238
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -362,6 +385,14 @@ public void write(TimeValuePair pair) {
logger.error("last query can't find storage group: path is: " + fullPath);
}
}
+
+ public boolean checkEmptyContainer() {
Review Comment:
```suggestion
@Override
public boolean checkEmptyContainer() {
```
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -220,15 +221,17 @@ public static List<Pair<Boolean, TimeValuePair>> calculateLastPairForSeriesLocal
resultContainer.get(i).right = readerList.get(index++).readLastPoint();
if (resultContainer.get(i).right.getValue() != null) {
resultContainer.get(i).left = true;
- if (CACHE_ENABLED) {
Review Comment:
Why delete the `if (CACHE_ENABLED)` statement?
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -220,15 +221,17 @@ public static List<Pair<Boolean, TimeValuePair>> calculateLastPairForSeriesLocal
resultContainer.get(i).right = readerList.get(index++).readLastPoint();
if (resultContainer.get(i).right.getValue() != null) {
resultContainer.get(i).left = true;
- if (CACHE_ENABLED) {
- cacheAccessors.get(i).write(resultContainer.get(i).right);
- if (context.isDebug()) {
- DEBUG_LOGGER.info(
- "[LastQueryExecutor] Update last cache for path: {} with timestamp: {}",
- seriesPaths,
- resultContainer.get(i).right.getTimestamp());
- }
+ cacheAccessors.get(i).write(resultContainer.get(i).right);
+ if (context.isDebug()) {
+ DEBUG_LOGGER.info(
+ "[LastQueryExecutor] Update last cache for path: {} with timestamp: {}",
+ seriesPaths,
+ resultContainer.get(i).right.getTimestamp());
}
+ } else {
+ resultContainer.get(i).left = true;
Review Comment:
It seems that there is no need to set resultContainer.get(i).left to true.
##########
integration/src/test/java/org/apache/iotdb/db/integration/IoTDBLastIT.java:
##########
@@ -576,6 +577,128 @@ public void lastWithFilterTest() {
}
}
+ // test cases that empty container would be created
+ @Test
+ public void lastWithEmptyContainer() {
+ String[] retArray =
+ new String[] {
+ "500,root.ln.wf01.wt02.temperature,15.7,DOUBLE",
+ "500,root.ln.wf01.wt02.status,false,BOOLEAN",
+ "500,root.ln.wf01.wt02.id,9,INT32",
+ "600,root.ln.wf01.wt02.temperature,10.2,DOUBLE",
+ "600,root.ln.wf01.wt02.status,false,BOOLEAN",
+ "600,root.ln.wf01.wt02.id,6,INT32"
+ };
+
+ try (Connection connection = EnvFactory.getEnv().getConnection();
+ Statement statement = connection.createStatement()) {
+
+ PartialPath path = new PartialPath("root.ln.wf01.wt02.temperature");
+ IoTDB.metaManager.resetLastCache(path);
+
+ IMeasurementMNode node;
+ node = IoTDB.metaManager.getMeasurementMNode(path);
Review Comment:
It's not an IT way, in IT tests, we only care about whether the external results are correct, we can never get the inner state of IoTDB which should be put into UT.
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -329,6 +335,23 @@ public void write(TimeValuePair pair) {
IoTDB.metaManager.updateLastCache(node, pair, false, Long.MIN_VALUE);
}
}
+
+ public boolean checkEmptyContainer() {
+ if (node == null) {
+ return false;
+ } else {
+ return node.getLastCacheContainer().isEmptyContainer();
+ }
+ }
+
+ public void markEmpty() {
+ if (node == null) {
+ return;
+ } else {
+ node.setLastCacheContainer(EmptyLastCacheContainer.getInstance());
Review Comment:
We can't directly use EmptyLastCacheContainer to set the node's LastCacheContainer because it may be already set to normal LastCacheContainer through another insert thread. If so, this operation may cause wrong cache container.
##########
integration/src/test/java/org/apache/iotdb/db/integration/IoTDBLastIT.java:
##########
@@ -576,6 +577,128 @@ public void lastWithFilterTest() {
}
}
+ // test cases that empty container would be created
+ @Test
+ public void lastWithEmptyContainer() {
+ String[] retArray =
+ new String[] {
+ "500,root.ln.wf01.wt02.temperature,15.7,DOUBLE",
+ "500,root.ln.wf01.wt02.status,false,BOOLEAN",
+ "500,root.ln.wf01.wt02.id,9,INT32",
+ "600,root.ln.wf01.wt02.temperature,10.2,DOUBLE",
+ "600,root.ln.wf01.wt02.status,false,BOOLEAN",
+ "600,root.ln.wf01.wt02.id,6,INT32"
+ };
+
+ try (Connection connection = EnvFactory.getEnv().getConnection();
+ Statement statement = connection.createStatement()) {
+
+ PartialPath path = new PartialPath("root.ln.wf01.wt02.temperature");
+ IoTDB.metaManager.resetLastCache(path);
+
+ IMeasurementMNode node;
+ node = IoTDB.metaManager.getMeasurementMNode(path);
+ Assert.assertTrue(node.getLastCacheContainer().isEmptyContainer());
+
+ boolean hasResultSet =
+ statement.execute("select last temperature,status,id from root.ln.wf01.wt02");
+
+ assertTrue(hasResultSet);
+ int cnt = 0;
+ try (ResultSet resultSet = statement.getResultSet()) {
+ while (resultSet.next()) {
+ String ans =
+ resultSet.getString(TIMESTAMP_STR)
+ + ","
+ + resultSet.getString(TIMESEIRES_STR)
+ + ","
+ + resultSet.getString(VALUE_STR)
+ + ","
+ + resultSet.getString(DATA_TYPE_STR);
+ Assert.assertEquals(retArray[cnt], ans);
+ cnt++;
+ }
+ }
+ } catch (Exception e) {
+ e.printStackTrace();
+ fail(e.getMessage());
+ }
+ try {
+ EnvironmentUtils.restartDaemon();
+ } catch (Exception e) {
+ Assert.fail();
+ }
+
+ try (Connection connection = EnvFactory.getEnv().getConnection();
+ Statement statement = connection.createStatement()) {
+
+ PartialPath path = new PartialPath("root.ln.wf01.wt02.temperature");
+ IMeasurementMNode node;
+ node = IoTDB.metaManager.getMeasurementMNode(path);
Review Comment:
same as above
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -362,6 +385,14 @@ public void write(TimeValuePair pair) {
logger.error("last query can't find storage group: path is: " + fullPath);
}
}
+
+ public boolean checkEmptyContainer() {
+ return false;
+ }
+
+ public void markEmpty() {
Review Comment:
```suggestion
@Override
public void markEmpty() {
```
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -244,23 +247,22 @@ private static List<Pair<Boolean, TimeValuePair>> readLastPairsFromCache(
List<TSDataType> restDataType,
boolean debugOn) {
List<Pair<Boolean, TimeValuePair>> resultContainer = new ArrayList<>();
- if (CACHE_ENABLED) {
- for (PartialPath path : seriesPaths) {
- if (ID_TABLE_ENABLED) {
- cacheAccessors.add(new IDTableLastCacheAccessor(path));
- } else {
- cacheAccessors.add(new MManagerLastCacheAccessor(path));
- }
- }
- } else {
- for (int i = 0; i < seriesPaths.size(); i++) {
- resultContainer.add(new Pair<>(false, null));
- PartialPath p = ((MeasurementPath) seriesPaths.get(i)).transformToExactPath();
- restPaths.add(p);
- restDataType.add(dataTypes.get(i));
+
+ for (PartialPath path : seriesPaths) {
+ if (ID_TABLE_ENABLED) {
+ cacheAccessors.add(new IDTableLastCacheAccessor(path));
+
+ } else {
+ cacheAccessors.add(new MManagerLastCacheAccessor(path));
}
}
+
Review Comment:
Why delete the `if (CACHE_ENABLED)`?
##########
integration/src/test/java/org/apache/iotdb/db/integration/IoTDBLastIT.java:
##########
@@ -576,6 +577,128 @@ public void lastWithFilterTest() {
}
}
+ // test cases that empty container would be created
+ @Test
+ public void lastWithEmptyContainer() {
+ String[] retArray =
+ new String[] {
+ "500,root.ln.wf01.wt02.temperature,15.7,DOUBLE",
+ "500,root.ln.wf01.wt02.status,false,BOOLEAN",
+ "500,root.ln.wf01.wt02.id,9,INT32",
+ "600,root.ln.wf01.wt02.temperature,10.2,DOUBLE",
+ "600,root.ln.wf01.wt02.status,false,BOOLEAN",
+ "600,root.ln.wf01.wt02.id,6,INT32"
+ };
+
+ try (Connection connection = EnvFactory.getEnv().getConnection();
+ Statement statement = connection.createStatement()) {
+
+ PartialPath path = new PartialPath("root.ln.wf01.wt02.temperature");
+ IoTDB.metaManager.resetLastCache(path);
+
+ IMeasurementMNode node;
+ node = IoTDB.metaManager.getMeasurementMNode(path);
+ Assert.assertTrue(node.getLastCacheContainer().isEmptyContainer());
+
+ boolean hasResultSet =
+ statement.execute("select last temperature,status,id from root.ln.wf01.wt02");
+
+ assertTrue(hasResultSet);
+ int cnt = 0;
+ try (ResultSet resultSet = statement.getResultSet()) {
+ while (resultSet.next()) {
+ String ans =
+ resultSet.getString(TIMESTAMP_STR)
+ + ","
+ + resultSet.getString(TIMESEIRES_STR)
+ + ","
+ + resultSet.getString(VALUE_STR)
+ + ","
+ + resultSet.getString(DATA_TYPE_STR);
+ Assert.assertEquals(retArray[cnt], ans);
+ cnt++;
+ }
+ }
+ } catch (Exception e) {
+ e.printStackTrace();
+ fail(e.getMessage());
+ }
+ try {
+ EnvironmentUtils.restartDaemon();
+ } catch (Exception e) {
+ Assert.fail();
+ }
+
+ try (Connection connection = EnvFactory.getEnv().getConnection();
+ Statement statement = connection.createStatement()) {
+
+ PartialPath path = new PartialPath("root.ln.wf01.wt02.temperature");
+ IMeasurementMNode node;
+ node = IoTDB.metaManager.getMeasurementMNode(path);
+ Assert.assertNull(node.getLastCacheContainer().getCachedLast());
+ boolean hasResultSet =
+ statement.execute("select last temperature,status,id from root.ln.wf01.wt02");
+ node = IoTDB.metaManager.getMeasurementMNode(path);
Review Comment:
same as above
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -329,6 +335,23 @@ public void write(TimeValuePair pair) {
IoTDB.metaManager.updateLastCache(node, pair, false, Long.MIN_VALUE);
}
}
+
+ public boolean checkEmptyContainer() {
+ if (node == null) {
+ return false;
+ } else {
+ return node.getLastCacheContainer().isEmptyContainer();
+ }
+ }
+
+ public void markEmpty() {
+ if (node == null) {
+ return;
+ } else {
+ node.setLastCacheContainer(EmptyLastCacheContainer.getInstance());
+ ;
Review Comment:
```suggestion
```
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -362,6 +385,14 @@ public void write(TimeValuePair pair) {
logger.error("last query can't find storage group: path is: " + fullPath);
}
}
+
+ public boolean checkEmptyContainer() {
Review Comment:
IDTable doesn't support EmptyContainer? Is it by design? @MarcosZyk
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] MarcosZyk commented on a diff in pull request #6442: [To rel/0.13][IOTDB-2525] Modify last cache value mechanism
Posted by GitBox <gi...@apache.org>.
MarcosZyk commented on code in PR #6442:
URL: https://github.com/apache/iotdb/pull/6442#discussion_r907939719
##########
server/src/main/java/org/apache/iotdb/db/metadata/lastCache/LastCacheManager.java:
##########
@@ -101,8 +107,7 @@ public static void resetLastCache(IMeasurementMNode node) {
checkIsTemplateLastCacheAndSetIfAbsent(node);
- ILastCacheContainer lastCacheContainer = node.getLastCacheContainer();
- lastCacheContainer.resetLastCache();
+ node.setLastCacheContainer(EmptyLastCacheContainer.getInstance());
Review Comment:
Add @TestOnly.
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -220,15 +221,16 @@ public static List<Pair<Boolean, TimeValuePair>> calculateLastPairForSeriesLocal
resultContainer.get(i).right = readerList.get(index++).readLastPoint();
if (resultContainer.get(i).right.getValue() != null) {
resultContainer.get(i).left = true;
- if (CACHE_ENABLED) {
- cacheAccessors.get(i).write(resultContainer.get(i).right);
- if (context.isDebug()) {
- DEBUG_LOGGER.info(
- "[LastQueryExecutor] Update last cache for path: {} with timestamp: {}",
- seriesPaths,
- resultContainer.get(i).right.getTimestamp());
- }
+ cacheAccessors.get(i).write(resultContainer.get(i).right);
+ if (context.isDebug()) {
+ DEBUG_LOGGER.info(
+ "[LastQueryExecutor] Update last cache for path: {} with timestamp: {}",
+ seriesPaths,
+ resultContainer.get(i).right.getTimestamp());
}
+ } else {
+ resultContainer.get(i).left = true;
+ cacheAccessors.get(i).markEmpty();
Review Comment:
Please add some comments to explain the whole process. It helps other people to understand the code.
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -244,23 +246,21 @@ private static List<Pair<Boolean, TimeValuePair>> readLastPairsFromCache(
List<TSDataType> restDataType,
boolean debugOn) {
List<Pair<Boolean, TimeValuePair>> resultContainer = new ArrayList<>();
- if (CACHE_ENABLED) {
- for (PartialPath path : seriesPaths) {
- if (ID_TABLE_ENABLED) {
- cacheAccessors.add(new IDTableLastCacheAccessor(path));
- } else {
- cacheAccessors.add(new MManagerLastCacheAccessor(path));
- }
- }
- } else {
- for (int i = 0; i < seriesPaths.size(); i++) {
- resultContainer.add(new Pair<>(false, null));
- PartialPath p = ((MeasurementPath) seriesPaths.get(i)).transformToExactPath();
- restPaths.add(p);
- restDataType.add(dataTypes.get(i));
+
+ for (PartialPath path : seriesPaths) {
+ if (ID_TABLE_ENABLED) {
+ cacheAccessors.add(new IDTableLastCacheAccessor(path));
+
+ } else {
+ cacheAccessors.add(new MManagerLastCacheAccessor(path));
}
}
+
for (int i = 0; i < cacheAccessors.size(); i++) {
+ if (cacheAccessors.get(i).checkEmptyContainer()) {
+ resultContainer.add(new Pair<>(true, null));
+ continue;
+ }
Review Comment:
Add some comments.
##########
server/src/main/java/org/apache/iotdb/db/metadata/lastCache/LastCacheManager.java:
##########
@@ -85,11 +87,15 @@ public static void updateLastCache(
checkIsTemplateLastCacheAndSetIfAbsent(node);
ILastCacheContainer lastCacheContainer = node.getLastCacheContainer();
+ if (lastCacheContainer.isEmptyContainer()) {
+ lastCacheContainer = new LastCacheContainer();
+ node.setLastCacheContainer(lastCacheContainer);
+ }
Review Comment:
This code block should be synchronized on the given node. Otherwise, multi threads will do the same operation, and this may lead to some of them losing execution results.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] yyt86 commented on a diff in pull request #6442: [To rel/0.13][IOTDB-2525] Modify last cache value mechanism
Posted by GitBox <gi...@apache.org>.
yyt86 commented on code in PR #6442:
URL: https://github.com/apache/iotdb/pull/6442#discussion_r916450425
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -329,6 +335,23 @@ public void write(TimeValuePair pair) {
IoTDB.metaManager.updateLastCache(node, pair, false, Long.MIN_VALUE);
}
}
+
+ public boolean checkEmptyContainer() {
+ if (node == null) {
+ return false;
+ } else {
+ return node.getLastCacheContainer().isEmptyContainer();
+ }
+ }
+
+ public void markEmpty() {
+ if (node == null) {
+ return;
+ } else {
+ node.setLastCacheContainer(EmptyLastCacheContainer.getInstance());
Review Comment:
Since the method `markEmpty` is called in the method `calculateLastPairForSeriesLocally` when the system restarts and the `LastCacheContainer` is initialized as null. So I think it may be safe to use `EmptyLastCacheContainer` to set the node's `LastCacheContainer`.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] MarcosZyk commented on a diff in pull request #6442: [To rel/0.13][IOTDB-2525] Modify last cache value mechanism
Posted by GitBox <gi...@apache.org>.
MarcosZyk commented on code in PR #6442:
URL: https://github.com/apache/iotdb/pull/6442#discussion_r915775984
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -362,6 +385,14 @@ public void write(TimeValuePair pair) {
logger.error("last query can't find storage group: path is: " + fullPath);
}
}
+
+ public boolean checkEmptyContainer() {
Review Comment:
Yes. Currently, we haven't consider IDTable.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [iotdb] yyt86 commented on a diff in pull request #6442: [To rel/0.13][IOTDB-2525] Modify last cache value mechanism
Posted by GitBox <gi...@apache.org>.
yyt86 commented on code in PR #6442:
URL: https://github.com/apache/iotdb/pull/6442#discussion_r916450425
##########
server/src/main/java/org/apache/iotdb/db/query/executor/LastQueryExecutor.java:
##########
@@ -329,6 +335,23 @@ public void write(TimeValuePair pair) {
IoTDB.metaManager.updateLastCache(node, pair, false, Long.MIN_VALUE);
}
}
+
+ public boolean checkEmptyContainer() {
+ if (node == null) {
+ return false;
+ } else {
+ return node.getLastCacheContainer().isEmptyContainer();
+ }
+ }
+
+ public void markEmpty() {
+ if (node == null) {
+ return;
+ } else {
+ node.setLastCacheContainer(EmptyLastCacheContainer.getInstance());
Review Comment:
Since the method `markEmpty` is called in the method `calculateLastPairForSeriesLocally` when the system restarts and the `LastCacheContainer` is initialized as null. So I think it may be safe to use `EmptyLastCacheContainer` to set the node's `LastCacheContainer`.
--
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: reviews-unsubscribe@iotdb.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org