JackieTien97 commented on a change in pull request #460:
URL: https://github.com/apache/iotdb/pull/460#discussion_r536686136
##########
File path: jdbc/src/main/java/org/apache/iotdb/jdbc/IoTDBStatement.java
##########
@@ -215,8 +215,8 @@ public boolean execute(String arg0, String[] arg1) throws
SQLException {
/**
* There are two kinds of sql here: (1) query sql (2) update sql.
- * (1) return IoTDBJDBCResultSet or IoTDBNonAlignJDBCResultSet
- * (2) simply get executed
+ * <p>
+ * (1) return IoTDBJDBCResultSet or IoTDBNonAlignJDBCResultSet (2) simply
get executed
Review comment:
format back
##########
File path: server/src/assembly/resources/conf/logback.xml
##########
@@ -52,7 +52,7 @@
<maxIndex>10</maxIndex>
</rollingPolicy>
<triggeringPolicy
class="ch.qos.logback.core.rolling.SizeBasedTriggeringPolicy">
- <maxFileSize>1MB</maxFileSize>
+ <maxFileSize>10MB</maxFileSize>
Review comment:
Why we need to change the default size of logs
##########
File path:
server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileResource.java
##########
@@ -279,12 +269,8 @@ public synchronized void serialize() throws IOException {
ReadWriteIOUtils.write(endTimes[entry.getValue()], outputStream);
}
- if (historicalVersions != null) {
- ReadWriteIOUtils.write(this.historicalVersions.size(), outputStream);
- for (Long historicalVersion : historicalVersions) {
- ReadWriteIOUtils.write(historicalVersion, outputStream);
- }
- }
+ ReadWriteIOUtils.write(maxPlanIndex, outputStream);
+ ReadWriteIOUtils.write(minPlanIndex, outputStream);
Review comment:
duplicated with the following lines
##########
File path:
server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileResource.java
##########
@@ -326,25 +312,18 @@ public void deserialize() throws IOException {
this.endTimes = endTimesArray;
this.deviceToIndex = deviceMap;
- if (inputStream.available() > 0) {
- int versionSize = ReadWriteIOUtils.readInt(inputStream);
- historicalVersions = new HashSet<>();
- for (int i = 0; i < versionSize; i++) {
- historicalVersions.add(ReadWriteIOUtils.readLong(inputStream));
- }
- } else {
- // use the version in file name as the historical version for files of
old versions
- long version =
Long.parseLong(file.getName().split(IoTDBConstant.FILE_NAME_SEPARATOR)[1]);
- historicalVersions = Collections.singleton(version);
- }
+ maxPlanIndex = ReadWriteIOUtils.readLong(inputStream);
+ minPlanIndex = ReadWriteIOUtils.readLong(inputStream);
Review comment:
same as above
##########
File path:
server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
##########
@@ -2079,16 +2055,21 @@ private void removeFullyOverlapFiles(TsFileResource
newTsFile, Iterator<TsFileRe
private void removeFullyOverlapFile(TsFileResource tsFileResource,
Iterator<TsFileResource> iterator
, boolean isSeq) {
+ logger.info("Removing a covered file {}, closed: {}", tsFileResource,
tsFileResource.isClosed());
if (!tsFileResource.isClosed()) {
- // also remove the TsFileProcessor if the overlapped file is not closed
- long timePartition = tsFileResource.getTimePartition();
- Map<Long, TsFileProcessor> fileProcessorMap = isSeq ?
workSequenceTsFileProcessors :
- workUnsequenceTsFileProcessors;
- TsFileProcessor tsFileProcessor = fileProcessorMap.get(timePartition);
- if (tsFileProcessor != null && tsFileProcessor.getTsFileResource() ==
tsFileResource) {
- //have to take some time to close the tsFileProcessor
- tsFileProcessor.syncClose();
- fileProcessorMap.remove(timePartition);
+ try {
+ // also remove the TsFileProcessor if the overlapped file is not closed
+ long timePartition = tsFileResource.getTimePartition();
+ Map<Long, TsFileProcessor> fileProcessorMap = isSeq ?
workSequenceTsFileProcessors :
+ workUnsequenceTsFileProcessors;
+ TsFileProcessor tsFileProcessor = fileProcessorMap.get(timePartition);
+ if (tsFileProcessor != null && tsFileProcessor.getTsFileResource() ==
tsFileResource) {
+ //have to take some time to close the tsFileProcessor
+ tsFileProcessor.syncClose();
+ fileProcessorMap.remove(timePartition);
+ }
+ } catch (Exception e) {
+ logger.error("Cannot close {}", tsFileResource, e);
Review comment:
Should we rethrow the exception and add a finally block to do the remove
things
##########
File path:
server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileResource.java
##########
@@ -585,20 +555,34 @@ public void removeModFile() throws IOException {
* Remove the data file, its resource file, and its modification file
physically.
*/
public void remove() {
- file.delete();
- fsFactory.getFile(file.getPath() + RESOURCE_SUFFIX).delete();
- fsFactory.getFile(file.getPath() + ModificationFile.FILE_SUFFIX).delete();
+ try {
+ Files.deleteIfExists(file.toPath());
+ } catch (IOException e) {
+ logger.warn("TsFile {} cannot be deleted: {}", file, e.getMessage());
Review comment:
better to rethrow the exception
##########
File path:
server/src/main/java/org/apache/iotdb/db/engine/compaction/no/NoCompactionTsFileManagement.java
##########
@@ -56,7 +56,7 @@ public NoCompactionTsFileManagement(String storageGroupName,
String storageGroup
if (sequence) {
return new ArrayList<>(sequenceFileTreeSet);
} else {
- return unSequenceFileList;
+ return new ArrayList<>(unSequenceFileList);
Review comment:
No need to copy it again here, it won't be changed outside.
Actually it's better that the interface's return type should be changed to
Collection
##########
File path: server/src/main/java/org/apache/iotdb/db/metadata/PartialPath.java
##########
@@ -20,24 +20,29 @@
import java.util.ArrayList;
import java.util.Arrays;
+import java.util.Collections;
import java.util.List;
import org.apache.iotdb.db.conf.IoTDBConstant;
import org.apache.iotdb.db.exception.metadata.IllegalPathException;
import org.apache.iotdb.db.utils.TestOnly;
import org.apache.iotdb.tsfile.common.constant.TsFileConstant;
import org.apache.iotdb.tsfile.read.common.Path;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* A prefix path, suffix path or fullPath generated from SQL.
* Usually used in the IoTDB server module
*/
public class PartialPath extends Path implements Comparable<Path> {
+ private static final Logger logger =
LoggerFactory.getLogger(PartialPath.class);
+
private String[] nodes;
// alias of measurement
- private String measurementAlias = null;
+ private String measurementAlias = "";
// alias of time series used in SELECT AS
- private String tsAlias = null;
+ private String tsAlias = "";
Review comment:
How do we distinguish whether the measurement has an empty string as its
alias or it doesn't have alias?
##########
File path:
server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileResource.java
##########
@@ -585,20 +555,34 @@ public void removeModFile() throws IOException {
* Remove the data file, its resource file, and its modification file
physically.
*/
public void remove() {
- file.delete();
- fsFactory.getFile(file.getPath() + RESOURCE_SUFFIX).delete();
- fsFactory.getFile(file.getPath() + ModificationFile.FILE_SUFFIX).delete();
+ try {
+ Files.deleteIfExists(file.toPath());
+ } catch (IOException e) {
+ logger.warn("TsFile {} cannot be deleted: {}", file, e.getMessage());
+ }
+ removeResourceFile();
+ try {
+ Files.deleteIfExists(
+ fsFactory.getFile(file.getPath() +
ModificationFile.FILE_SUFFIX).toPath());
+ } catch (IOException e) {
+ logger.warn("ModificationFile {} cannot be deleted: {}", file,
e.getMessage());
Review comment:
same as above
##########
File path:
server/src/main/java/org/apache/iotdb/db/engine/storagegroup/TsFileResource.java
##########
@@ -585,20 +555,34 @@ public void removeModFile() throws IOException {
* Remove the data file, its resource file, and its modification file
physically.
*/
public void remove() {
- file.delete();
- fsFactory.getFile(file.getPath() + RESOURCE_SUFFIX).delete();
- fsFactory.getFile(file.getPath() + ModificationFile.FILE_SUFFIX).delete();
+ try {
+ Files.deleteIfExists(file.toPath());
+ } catch (IOException e) {
+ logger.warn("TsFile {} cannot be deleted: {}", file, e.getMessage());
+ }
+ removeResourceFile();
+ try {
+ Files.deleteIfExists(
+ fsFactory.getFile(file.getPath() +
ModificationFile.FILE_SUFFIX).toPath());
+ } catch (IOException e) {
+ logger.warn("ModificationFile {} cannot be deleted: {}", file,
e.getMessage());
+ }
}
public void removeResourceFile() {
- fsFactory.getFile(file.getPath() + RESOURCE_SUFFIX).delete();
+ try {
+ Files.deleteIfExists(fsFactory.getFile(file.getPath() +
RESOURCE_SUFFIX).toPath());
+ } catch (IOException e) {
+ logger.warn("TsFileResource {} cannot be deleted: {}", file,
e.getMessage());
Review comment:
same as above
##########
File path:
tsfile/src/main/java/org/apache/iotdb/tsfile/read/reader/page/PageReader.java
##########
@@ -162,7 +163,11 @@ public Statistics getStatistics() {
@Override
public void setFilter(Filter filter) {
- this.filter = filter;
+ if (this.filter == null) {
Review comment:
Actually, i think we should just use the filter to overwrite the former
one. The former one is actually a time filter and it has been checked when the
PageReader is constructed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
