-
Notifications
You must be signed in to change notification settings - Fork 976
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[IOTDB-884] group createMultiTimeseriesPlan by partitionGroup #1854
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great Work!
Only minor issues~
} else if (plan instanceof CreateMultiTimeSeriesPlan) { | ||
// CreateMultiTimeSeriesPlans contain many rows, each will correspond to a TSStatus as its | ||
// execution result, as the plan is split and the sub-plans may have interleaving ranges, | ||
// we must assure that each TSStatus is placed to the right position |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor Suggesion:
// CreateMultiTimeSeriesPlan contains many rows, each will correspond to a TSStatus as its
// execution result, as the plan is splited and the sub-plans may have interleaving ranges,
// we must assure that each TSStatus is placed to the right position
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
|
||
plan.setResults(results); | ||
if (noFailure || isBatchFailure) { | ||
return RpcUtils.getStatus(TSStatusCode.SUCCESS_STATUS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use StatusUtils.OK
? then we can reduce a creation of a TSStatus object with the same meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it!
logger.debug("{}: from {},{},{}", name, entry.getKey(), entry.getValue(), tmpStatus); | ||
noFailure = | ||
(tmpStatus.getCode() == TSStatusCode.SUCCESS_STATUS.getStatusCode()) && noFailure; | ||
isBatchFailure = (tmpStatus.getCode() == TSStatusCode.MULTIPLE_ERROR.getStatusCode()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When will forwardToSingleGroup
return a TSStatusCode.MULTIPLE_ERROR during creating multi timeseries? It seems that this TSStatus will only be returned by BatchInsertionException, which is only thrown during 'insertTablet' interface currently~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you are right! I'll fix it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job, but please see to these problems.
cluster/src/main/java/org/apache/iotdb/cluster/query/ClusterPlanRouter.java
Show resolved
Hide resolved
cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
Outdated
Show resolved
Hide resolved
cluster/src/main/java/org/apache/iotdb/cluster/server/member/MetaGroupMember.java
Outdated
Show resolved
Hide resolved
|
||
@Override | ||
public void removeHardLink(String hardLinkPath, AsyncMethodCallback<Void> resultHandler) throws TException { | ||
System.out.println("remote hardLinkPath: " + hardLinkPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually delete the file using something like Files.deleteIfExists()
.
@@ -1937,6 +1937,7 @@ private void removeFullyOverlapFiles(TsFileResource newTsFile, Iterator<TsFileRe | |||
while (iterator.hasNext()) { | |||
TsFileResource existingTsFile = iterator.next(); | |||
if (newTsFile.getHistoricalVersions().containsAll(existingTsFile.getHistoricalVersions()) | |||
&& !newTsFile.getHistoricalVersions().equals(existingTsFile.getHistoricalVersions()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may skip the unclosed TsFile that should be replaced by the pulled file, and I think cluster_new now has properly handled this, so please abort the change.
results = new TSStatus[createMultiTimeSeriesPlan.getIndexes().size()]; | ||
Arrays.fill(results, RpcUtils.SUCCESS_STATUS); | ||
} | ||
results[i] = RpcUtils.getStatus(TSStatusCode.INTERNAL_SERVER_ERROR, e.getMessage()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use e.getErrorCode()
here.
createMultiTimeSeriesPlan.setResults(results); | ||
|
||
if (hasFailed) { | ||
throw new BatchInsertionException(results); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should rename this to BatchProcessException
now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
https://issues.apache.org/jira/browse/IOTDB-884