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/03/07 06:32:16 UTC
[GitHub] [iotdb] ericpai opened a new pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
ericpai opened a new pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173
See JIRA: https://issues.apache.org/jira/browse/IOTDB-2692
--
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] ericpai commented on a change in pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#discussion_r820684402
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/inner/utils/InnerSpaceCompactionUtils.java
##########
@@ -108,12 +112,22 @@ private static void compactNotAlignedSeries(
while (seriesIterator.hasNextSeries()) {
checkThreadInterrupted(targetResource);
// TODO: we can provide a configuration item to enable concurrent between each series
- String currentSeries = seriesIterator.nextSeries();
+ PartialPath p = new PartialPath(device, seriesIterator.nextSeries());
Review comment:
The logic of constructing the full timeseries path is not changed.
Besides, the data of `seriesIterator` comes from https://github.com/apache/iotdb/blob/master/tsfile/src/main/java/org/apache/iotdb/tsfile/read/TsFileSequenceReader.java#L1896-L1931 , which returns a map with keys of type `TimeseriesMetaData.getMeasurementId()`.
--
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] coveralls edited a comment on pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#issuecomment-1060269674
[](https://coveralls.io/builds/47127290)
Coverage decreased (-0.02%) to 67.813% when pulling **28e080df3ca17b2b65598f4da18e125eb02b2176 on ericpai:bugfix/iotdb-2692** into **f9b16cd19f84523ed3ffdbf65df065732cec3b77 on apache:master**.
--
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] coveralls edited a comment on pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#issuecomment-1060269674
[](https://coveralls.io/builds/47120407)
Coverage increased (+0.008%) to 67.782% when pulling **e51969c99f40ce3364b45fc608459e87cb518639 on ericpai:bugfix/iotdb-2692** into **6d15dd0ebd11f170f6918d04fb94a5d8e6a56f38 on apache:master**.
--
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] ericpai commented on pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
ericpai commented on pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#issuecomment-1060239235
@THUMarkLau @choubenson PTAL
--
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] qiaojialin commented on a change in pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
qiaojialin commented on a change in pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#discussion_r820446050
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/CompactionUtils.java
##########
@@ -125,8 +125,12 @@ private static void compactAlignedSeries(
List<IMeasurementSchema> measurementSchemas = new ArrayList<>();
for (String measurement : allMeasurments) {
// TODO: use IDTable
- measurementSchemas.add(
- IoTDB.metaManager.getSeriesSchema(new PartialPath(device, measurement)));
+ PartialPath p = new PartialPath(device, measurement);
+ if (!IoTDB.metaManager.isPathExist(p)) {
+ logger.info("As path {} is dropped we skip it", p.getFullPath());
+ continue;
+ }
Review comment:
Hi, what if the deletion just takes effect between the check and the getSeriesSchema?
--
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] coveralls commented on pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#issuecomment-1060269674
[](https://coveralls.io/builds/47115151)
Coverage increased (+0.04%) to 67.814% when pulling **3d343a2e194c86790da57ab6490c0792044637c4 on ericpai:bugfix/iotdb-2692** into **6d15dd0ebd11f170f6918d04fb94a5d8e6a56f38 on apache:master**.
--
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] ericpai commented on a change in pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#discussion_r821367808
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/CompactionUtils.java
##########
@@ -121,20 +123,30 @@ private static void compactAlignedSeries(
throws IOException, MetadataException {
MultiTsFileDeviceIterator.AlignedMeasurmentIterator alignedMeasurmentIterator =
deviceIterator.iterateAlignedSeries(device);
- List<String> allMeasurments = alignedMeasurmentIterator.getAllMeasurements();
+ List<String> allMeasurements = alignedMeasurmentIterator.getAllMeasurements();
List<IMeasurementSchema> measurementSchemas = new ArrayList<>();
- for (String measurement : allMeasurments) {
+ for (String measurement : allMeasurements) {
// TODO: use IDTable
- measurementSchemas.add(
- IoTDB.metaManager.getSeriesSchema(new PartialPath(device, measurement)));
+ try {
+ measurementSchemas.add(
Review comment:
IMO, we can category the timing of deleting timeseries and compaction:
1. If the timeseries deletion happens before compaction, all the closed tsfiles will skip those deleted timeseries data and the merged one will not contain them.
2. If the timeseries deletion happens during compaction, as it has already checked the measurements validation before compaction, the deleted timeseries data is still merged in this compaction work. Although it's a waste of the resource, but no program or data error happens. And in next compaction task, the former deleted data will be processed as condition 1.
--
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] THUMarkLau commented on a change in pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
THUMarkLau commented on a change in pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#discussion_r821305320
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/CompactionUtils.java
##########
@@ -121,20 +123,30 @@ private static void compactAlignedSeries(
throws IOException, MetadataException {
MultiTsFileDeviceIterator.AlignedMeasurmentIterator alignedMeasurmentIterator =
deviceIterator.iterateAlignedSeries(device);
- List<String> allMeasurments = alignedMeasurmentIterator.getAllMeasurements();
+ List<String> allMeasurements = alignedMeasurmentIterator.getAllMeasurements();
List<IMeasurementSchema> measurementSchemas = new ArrayList<>();
- for (String measurement : allMeasurments) {
+ for (String measurement : allMeasurements) {
// TODO: use IDTable
- measurementSchemas.add(
- IoTDB.metaManager.getSeriesSchema(new PartialPath(device, measurement)));
+ try {
+ measurementSchemas.add(
Review comment:
What if the series get deleted when compacting aligned series in inner space?
--
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] ericpai commented on pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
ericpai commented on pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#issuecomment-1060380375
> Please try to add an UT
Thanks for your comments! I have updated the existence test cases to cover the situation that timeseries are deleted before compaction.
--
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] ericpai commented on a change in pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#discussion_r820679542
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/inner/utils/InnerSpaceCompactionUtils.java
##########
@@ -108,12 +112,22 @@ private static void compactNotAlignedSeries(
while (seriesIterator.hasNextSeries()) {
checkThreadInterrupted(targetResource);
// TODO: we can provide a configuration item to enable concurrent between each series
- String currentSeries = seriesIterator.nextSeries();
+ PartialPath p = new PartialPath(device, seriesIterator.nextSeries());
+ IMeasurementSchema ms;
Review comment:
Fixed.
--
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] ericpai commented on a change in pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#discussion_r820515118
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/CompactionUtils.java
##########
@@ -125,8 +125,12 @@ private static void compactAlignedSeries(
List<IMeasurementSchema> measurementSchemas = new ArrayList<>();
for (String measurement : allMeasurments) {
// TODO: use IDTable
- measurementSchemas.add(
- IoTDB.metaManager.getSeriesSchema(new PartialPath(device, measurement)));
+ PartialPath p = new PartialPath(device, measurement);
+ if (!IoTDB.metaManager.isPathExist(p)) {
+ logger.info("As path {} is dropped we skip it", p.getFullPath());
+ continue;
+ }
Review comment:
`try-catch` is added instead of pre-check.
--
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] qiaojialin commented on a change in pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
qiaojialin commented on a change in pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#discussion_r820464415
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/CompactionUtils.java
##########
@@ -125,8 +125,12 @@ private static void compactAlignedSeries(
List<IMeasurementSchema> measurementSchemas = new ArrayList<>();
for (String measurement : allMeasurments) {
// TODO: use IDTable
- measurementSchemas.add(
- IoTDB.metaManager.getSeriesSchema(new PartialPath(device, measurement)));
+ PartialPath p = new PartialPath(device, measurement);
+ if (!IoTDB.metaManager.isPathExist(p)) {
+ logger.info("As path {} is dropped we skip it", p.getFullPath());
+ continue;
+ }
Review comment:
Yes, I prefer to just catch the exception and ignore it
--
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] ericpai merged pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
ericpai merged pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173
--
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] ericpai commented on a change in pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
ericpai commented on a change in pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#discussion_r820446976
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/CompactionUtils.java
##########
@@ -125,8 +125,12 @@ private static void compactAlignedSeries(
List<IMeasurementSchema> measurementSchemas = new ArrayList<>();
for (String measurement : allMeasurments) {
// TODO: use IDTable
- measurementSchemas.add(
- IoTDB.metaManager.getSeriesSchema(new PartialPath(device, measurement)));
+ PartialPath p = new PartialPath(device, measurement);
+ if (!IoTDB.metaManager.isPathExist(p)) {
+ logger.info("As path {} is dropped we skip it", p.getFullPath());
+ continue;
+ }
Review comment:
That's a good catch, maybe we can just catch the exception without pre existence check?
--
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] qiaojialin commented on a change in pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
qiaojialin commented on a change in pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#discussion_r820636535
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/inner/utils/InnerSpaceCompactionUtils.java
##########
@@ -108,12 +112,22 @@ private static void compactNotAlignedSeries(
while (seriesIterator.hasNextSeries()) {
checkThreadInterrupted(targetResource);
// TODO: we can provide a configuration item to enable concurrent between each series
- String currentSeries = seriesIterator.nextSeries();
+ PartialPath p = new PartialPath(device, seriesIterator.nextSeries());
+ IMeasurementSchema ms;
Review comment:
```suggestion
IMeasurementSchema measurementSchema;
```
##########
File path: server/src/main/java/org/apache/iotdb/db/engine/compaction/inner/utils/InnerSpaceCompactionUtils.java
##########
@@ -108,12 +112,22 @@ private static void compactNotAlignedSeries(
while (seriesIterator.hasNextSeries()) {
checkThreadInterrupted(targetResource);
// TODO: we can provide a configuration item to enable concurrent between each series
- String currentSeries = seriesIterator.nextSeries();
+ PartialPath p = new PartialPath(device, seriesIterator.nextSeries());
Review comment:
I'm not sure if this seriesIterator.nextSeries() returns a measurementId or a time series fullPath, where fullPath = deviceId.measurmentId.
If it is a fullPath, we will get a deviceId.deviceId.measurementId.
--
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] coveralls edited a comment on pull request #5173: [IOTDB-2692] Fix compaction exception caused by deleted timeseries
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #5173:
URL: https://github.com/apache/iotdb/pull/5173#issuecomment-1060269674
[](https://coveralls.io/builds/47127298)
Coverage increased (+0.003%) to 67.833% when pulling **28e080df3ca17b2b65598f4da18e125eb02b2176 on ericpai:bugfix/iotdb-2692** into **f9b16cd19f84523ed3ffdbf65df065732cec3b77 on apache:master**.
--
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