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 2020/05/20 05:47:25 UTC
[GitHub] [incubator-iotdb] qiaojialin commented on a change in pull request #1169: Premerge for the distributed version
qiaojialin commented on a change in pull request #1169:
URL: https://github.com/apache/incubator-iotdb/pull/1169#discussion_r426386618
##########
File path: session/src/main/java/org/apache/iotdb/session/Session.java
##########
@@ -105,7 +107,7 @@ public Session(String host, int port, String username, String password, int fetc
}
public synchronized void open() throws IoTDBConnectionException {
- open(false, Config.DEFAULT_TIMEOUT_MS);
+ open(true, Config.DEFAULT_TIMEOUT_MS);
Review comment:
there is a field enableRPCCompression, better to change it to true
##########
File path: server/src/main/java/org/apache/iotdb/db/auth/user/BasicUserManager.java
##########
@@ -310,4 +311,23 @@ public void setUserUseWaterMark(String username, boolean useWaterMark) throws Au
throw new AuthException(e);
}
}
+
+
+ @Override
+ public void replaceAllUsers(Map<String, User> users) throws AuthException {
+ synchronized (this) {
Review comment:
the same with BasicRoleManager
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/modification/ModificationFile.java
##########
@@ -125,4 +133,34 @@ public void remove() throws IOException {
FSFactoryProducer.getFSFactory().getFile(filePath).delete();
}
+ public boolean exists() {
+ return new File(filePath).exists();
+ }
+
+ /**
+ * Create a hardlink for the modification file.
+ * The hardlink with have a suffix like ".{sysTime}_{randomLong}"
+ * @return a new ModificationFile with its path changed to the hardlink, or null if the origin
+ * file does not exist or the hardlink cannot be created.
+ */
+ public ModificationFile createHardlink() {
+ if (!exists()) {
+ return null;
+ }
+
+ while (true) {
+ String hardlinkSuffix = "." + System.currentTimeMillis() + "_" + random.nextLong();
+ File hardlink = new File(filePath + hardlinkSuffix);
+
+ try {
+ Files.createLink(Paths.get(hardlink.getAbsolutePath()), Paths.get(filePath));
+ return new ModificationFile(hardlink.getAbsolutePath());
+ } catch (FileAlreadyExistsException e) {
+ // retry a different name if the file is already created
Review comment:
retry?
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
##########
@@ -1875,35 +1918,52 @@ public void removeFullyOverlapFiles(TsFileResource resource) {
closeQueryLock.writeLock().lock();
try {
Iterator<TsFileResource> iterator = sequenceFileTreeSet.iterator();
- removeFullyOverlapFiles(resource, iterator);
+ removeFullyOverlapFiles(resource, iterator, true);
iterator = unSequenceFileList.iterator();
- removeFullyOverlapFiles(resource, iterator);
+ removeFullyOverlapFiles(resource, iterator, false);
} finally {
closeQueryLock.writeLock().unlock();
writeUnlock();
}
}
- private void removeFullyOverlapFiles(TsFileResource resource, Iterator<TsFileResource> iterator) {
+ private void removeFullyOverlapFiles(TsFileResource newTsFile, Iterator<TsFileResource> iterator
+ , boolean isSeq) {
while (iterator.hasNext()) {
- TsFileResource seqFile = iterator.next();
- if (resource.getHistoricalVersions().containsAll(seqFile.getHistoricalVersions())
- && !resource.getHistoricalVersions().equals(seqFile.getHistoricalVersions())
- && seqFile.getWriteQueryLock().writeLock().tryLock()) {
+ TsFileResource existingTsFile = iterator.next();
+ if (newTsFile.getHistoricalVersions().containsAll(existingTsFile.getHistoricalVersions())
+ && !newTsFile.getHistoricalVersions().equals(existingTsFile.getHistoricalVersions())
+ && existingTsFile.tryWriteLock()) {
try {
- iterator.remove();
- seqFile.remove();
+ removeFullyOverlapFile(existingTsFile, iterator, isSeq);
} catch (Exception e) {
logger.error("Something gets wrong while removing FullyOverlapFiles ", e);
throw e;
} finally {
- seqFile.getWriteQueryLock().writeLock().unlock();
+ existingTsFile.writeUnlock();
}
}
}
}
+ private void removeFullyOverlapFile(TsFileResource tsFileResource, Iterator<TsFileResource> iterator
+ , boolean isSeq) {
+ 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) {
+ tsFileProcessor.syncClose();
Review comment:
this may take a long time, better add javadoc
##########
File path: server/src/main/java/org/apache/iotdb/db/qp/executor/PlanExecutor.java
##########
@@ -322,7 +323,13 @@ protected QueryDataSet processDataQuery(QueryPlan queryPlan, QueryContext contex
return queryDataSet;
}
- private QueryDataSet processShowQuery(ShowPlan showPlan)
+ @SuppressWarnings("unused")
+ protected AlignByDeviceDataSet getAlignByDeviceDataSet(AlignByDevicePlan plan,
Review comment:
it is obviously used...
##########
File path: server/src/main/java/org/apache/iotdb/db/auth/role/BasicRoleManager.java
##########
@@ -167,4 +168,21 @@ public void reset() {
rtlist.sort(null);
return rtlist;
}
+
+ @Override
+ public void replaceAllRoles(Map<String, Role> roles) throws AuthException {
+ synchronized (this) {
Review comment:
in other methods the lock is used, but here synchronized this object, is there a concurrent error when using accessor?
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/storagegroup/StorageGroupProcessor.java
##########
@@ -1875,35 +1918,52 @@ public void removeFullyOverlapFiles(TsFileResource resource) {
closeQueryLock.writeLock().lock();
try {
Iterator<TsFileResource> iterator = sequenceFileTreeSet.iterator();
- removeFullyOverlapFiles(resource, iterator);
+ removeFullyOverlapFiles(resource, iterator, true);
iterator = unSequenceFileList.iterator();
- removeFullyOverlapFiles(resource, iterator);
+ removeFullyOverlapFiles(resource, iterator, false);
} finally {
closeQueryLock.writeLock().unlock();
writeUnlock();
}
}
- private void removeFullyOverlapFiles(TsFileResource resource, Iterator<TsFileResource> iterator) {
+ private void removeFullyOverlapFiles(TsFileResource newTsFile, Iterator<TsFileResource> iterator
+ , boolean isSeq) {
while (iterator.hasNext()) {
- TsFileResource seqFile = iterator.next();
- if (resource.getHistoricalVersions().containsAll(seqFile.getHistoricalVersions())
- && !resource.getHistoricalVersions().equals(seqFile.getHistoricalVersions())
- && seqFile.getWriteQueryLock().writeLock().tryLock()) {
+ TsFileResource existingTsFile = iterator.next();
+ if (newTsFile.getHistoricalVersions().containsAll(existingTsFile.getHistoricalVersions())
+ && !newTsFile.getHistoricalVersions().equals(existingTsFile.getHistoricalVersions())
+ && existingTsFile.tryWriteLock()) {
Review comment:
how do we handle if trylock failed? ignore?
----------------------------------------------------------------
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:
users@infra.apache.org