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/10/28 08:15:21 UTC
[GitHub] [iotdb] yschengzi opened a new pull request, #7776: [IOTDB-4676][IOTDB-4677] Support sync DeleteTimeseries and sync Delete Data
yschengzi opened a new pull request, #7776:
URL: https://github.com/apache/iotdb/pull/7776
- Support collect DeleteTimeseries in sender
- Support load DeleteTimeseries in receiver
--
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] neuyilan commented on a diff in pull request #7776: [IOTDB-4676][IOTDB-4677] Support sync DeleteTimeseries and sync Delete Data
Posted by GitBox <gi...@apache.org>.
neuyilan commented on code in PR #7776:
URL: https://github.com/apache/iotdb/pull/7776#discussion_r1009115079
##########
server/src/main/java/org/apache/iotdb/db/sync/sender/manager/LocalSyncManager.java:
##########
@@ -46,8 +54,31 @@ public LocalSyncManager(DataRegion dataRegion, Pipe pipe) {
/** tsfile */
@Override
- public void syncRealTimeDeletion(Deletion deletion) {
- syncPipe.collectRealTimeDeletion(deletion, dataRegion.getStorageGroupName(), dataRegionId);
+ public void syncRealTimeDeletion(
+ Deletion deletion,
+ List<TsFileResource> unsealedResources,
+ List<TsFileResource> sealedResources) {
+ Set<String> devices = new HashSet<>();
+ unsealedResources.forEach(o -> devices.addAll(o.getDevices()));
+ sealedResources.forEach(o -> devices.addAll(o.getDevices()));
+
+ for (String device : devices) {
+ try {
+ PartialPath path =
+ new PartialPath(device).concatNode(IoTDBConstant.MULTI_LEVEL_PATH_WILDCARD);
+ if (deletion.getPath().overlapWith(path)) {
+ Deletion splitDeletion =
+ new Deletion(deletion.getPath(), 0, deletion.getStartTime(), deletion.getEndTime());
+ syncPipe.collectRealTimeDeletion(
+ splitDeletion, dataRegion.getStorageGroupName(), dataRegionId);
+ }
+ } catch (IllegalPathException e) {
+ logger.error(
+ String.format(
+ "Parse device path %s to partial path error when splitting deletion.", device),
+ e);
Review Comment:
These codes may not really be understandable,
If the codes in 69 is true for `device1`, then the deletion will be added to the sync pipe.
In the next loop, for `device2`, if the codes in 69 is still `TRUE`, then the deletion will be added to the sync pipe **again**. is this expected?
--
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] Cpaulyz commented on a diff in pull request #7776: [IOTDB-4676][IOTDB-4677] Support sync DeleteTimeseries and sync Delete Data
Posted by GitBox <gi...@apache.org>.
Cpaulyz commented on code in PR #7776:
URL: https://github.com/apache/iotdb/pull/7776#discussion_r1008248639
##########
server/src/main/java/org/apache/iotdb/db/sync/sender/manager/LocalSyncManager.java:
##########
@@ -46,8 +53,30 @@ public LocalSyncManager(DataRegion dataRegion, Pipe pipe) {
/** tsfile */
@Override
- public void syncRealTimeDeletion(Deletion deletion) {
- syncPipe.collectRealTimeDeletion(deletion, dataRegion.getStorageGroupName(), dataRegionId);
+ public void syncRealTimeDeletion(
+ Deletion deletion,
+ List<TsFileResource> unsealedResources,
+ List<TsFileResource> sealedResources) {
+ Set<String> devices = new HashSet<>();
+ unsealedResources.forEach(o -> devices.addAll(o.getDevices()));
+ sealedResources.forEach(o -> devices.addAll(o.getDevices()));
+
+ for (String device : devices) {
+ try {
+ PartialPath path = new PartialPath(device);
+ if (deletion.getPath().overlapWith(path)) {
+ Deletion splitDeletion =
+ new Deletion(path, 0, deletion.getStartTime(), deletion.getEndTime());
Review Comment:
When deletion.getPath() is a full path without wildcards, the splitting logic may be error.
e.g. "root.sg.d1.s1".overlapWith("root.sg.d1") is false.
--
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] yschengzi commented on a diff in pull request #7776: [IOTDB-4676][IOTDB-4677] Support sync DeleteTimeseries and sync Delete Data
Posted by GitBox <gi...@apache.org>.
yschengzi commented on code in PR #7776:
URL: https://github.com/apache/iotdb/pull/7776#discussion_r1019793023
##########
server/src/main/java/org/apache/iotdb/db/sync/sender/manager/LocalSyncManager.java:
##########
@@ -46,8 +54,36 @@ public LocalSyncManager(DataRegion dataRegion, Pipe pipe) {
/** tsfile */
@Override
- public void syncRealTimeDeletion(Deletion deletion) {
- syncPipe.collectRealTimeDeletion(deletion, dataRegion.getStorageGroupName(), dataRegionId);
+ public void syncRealTimeDeletion(
+ Deletion deletion,
+ List<TsFileResource> unsealedResources,
+ List<TsFileResource> sealedResources) {
Review Comment:
done.
--
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] Cpaulyz commented on a diff in pull request #7776: [IOTDB-4676][IOTDB-4677] Support sync DeleteTimeseries and sync Delete Data
Posted by GitBox <gi...@apache.org>.
Cpaulyz commented on code in PR #7776:
URL: https://github.com/apache/iotdb/pull/7776#discussion_r1009213846
##########
server/src/main/java/org/apache/iotdb/db/sync/sender/manager/LocalSyncManager.java:
##########
@@ -46,8 +54,31 @@ public LocalSyncManager(DataRegion dataRegion, Pipe pipe) {
/** tsfile */
@Override
- public void syncRealTimeDeletion(Deletion deletion) {
- syncPipe.collectRealTimeDeletion(deletion, dataRegion.getStorageGroupName(), dataRegionId);
+ public void syncRealTimeDeletion(
+ Deletion deletion,
+ List<TsFileResource> unsealedResources,
+ List<TsFileResource> sealedResources) {
+ Set<String> devices = new HashSet<>();
+ unsealedResources.forEach(o -> devices.addAll(o.getDevices()));
+ sealedResources.forEach(o -> devices.addAll(o.getDevices()));
+
+ for (String device : devices) {
+ try {
+ PartialPath path =
+ new PartialPath(device).concatNode(IoTDBConstant.MULTI_LEVEL_PATH_WILDCARD);
+ if (deletion.getPath().overlapWith(path)) {
+ Deletion splitDeletion =
+ new Deletion(deletion.getPath(), 0, deletion.getStartTime(), deletion.getEndTime());
+ syncPipe.collectRealTimeDeletion(
+ splitDeletion, dataRegion.getStorageGroupName(), dataRegionId);
+ }
+ } catch (IllegalPathException e) {
+ logger.error(
+ String.format(
+ "Parse device path %s to partial path error when splitting deletion.", device),
+ e);
Review Comment:
> These codes may not really be understandable,
>
> If the codes in 69 is true for `device1`, then the deletion will be added to the sync pipe.
>
> In the next loop, for `device2`, if the codes in 69 is still `TRUE`, then the deletion will be added to the sync pipe **again**. is this expected?
Yes.. The implementation is incorrect and still WIP. We should put the intersection of `device+".*"` and `deletion.getPath()` to splitDeletion.（#7823）
--
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] Cpaulyz commented on a diff in pull request #7776: [IOTDB-4676][IOTDB-4677] Support sync DeleteTimeseries and sync Delete Data
Posted by GitBox <gi...@apache.org>.
Cpaulyz commented on code in PR #7776:
URL: https://github.com/apache/iotdb/pull/7776#discussion_r1019782903
##########
server/src/main/java/org/apache/iotdb/db/sync/sender/manager/LocalSyncManager.java:
##########
@@ -46,8 +54,36 @@ public LocalSyncManager(DataRegion dataRegion, Pipe pipe) {
/** tsfile */
@Override
- public void syncRealTimeDeletion(Deletion deletion) {
- syncPipe.collectRealTimeDeletion(deletion, dataRegion.getStorageGroupName(), dataRegionId);
+ public void syncRealTimeDeletion(
+ Deletion deletion,
+ List<TsFileResource> unsealedResources,
+ List<TsFileResource> sealedResources) {
Review Comment:
To avoid affecting the deletion performance, we should return directly if syncDelOp=false.
Maybe add a getter in TsFilePipe to get syncDelOp.
--
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] Cpaulyz commented on a diff in pull request #7776: [IOTDB-4676][IOTDB-4677] Support sync DeleteTimeseries and sync Delete Data
Posted by GitBox <gi...@apache.org>.
Cpaulyz commented on code in PR #7776:
URL: https://github.com/apache/iotdb/pull/7776#discussion_r1009213846
##########
server/src/main/java/org/apache/iotdb/db/sync/sender/manager/LocalSyncManager.java:
##########
@@ -46,8 +54,31 @@ public LocalSyncManager(DataRegion dataRegion, Pipe pipe) {
/** tsfile */
@Override
- public void syncRealTimeDeletion(Deletion deletion) {
- syncPipe.collectRealTimeDeletion(deletion, dataRegion.getStorageGroupName(), dataRegionId);
+ public void syncRealTimeDeletion(
+ Deletion deletion,
+ List<TsFileResource> unsealedResources,
+ List<TsFileResource> sealedResources) {
+ Set<String> devices = new HashSet<>();
+ unsealedResources.forEach(o -> devices.addAll(o.getDevices()));
+ sealedResources.forEach(o -> devices.addAll(o.getDevices()));
+
+ for (String device : devices) {
+ try {
+ PartialPath path =
+ new PartialPath(device).concatNode(IoTDBConstant.MULTI_LEVEL_PATH_WILDCARD);
+ if (deletion.getPath().overlapWith(path)) {
+ Deletion splitDeletion =
+ new Deletion(deletion.getPath(), 0, deletion.getStartTime(), deletion.getEndTime());
+ syncPipe.collectRealTimeDeletion(
+ splitDeletion, dataRegion.getStorageGroupName(), dataRegionId);
+ }
+ } catch (IllegalPathException e) {
+ logger.error(
+ String.format(
+ "Parse device path %s to partial path error when splitting deletion.", device),
+ e);
Review Comment:
> These codes may not really be understandable,
>
> If the codes in 69 is true for `device1`, then the deletion will be added to the sync pipe.
>
> In the next loop, for `device2`, if the codes in 69 is still `TRUE`, then the deletion will be added to the sync pipe **again**. is this expected?
Yes.. The implementation is incorrect and still WIP. We should put the intersection of `device+".*"` and `deletion.getPath()` to splitDeletion.（#7823）
--
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] yschengzi commented on a diff in pull request #7776: [IOTDB-4676][IOTDB-4677] Support sync DeleteTimeseries and sync Delete Data
Posted by GitBox <gi...@apache.org>.
yschengzi commented on code in PR #7776:
URL: https://github.com/apache/iotdb/pull/7776#discussion_r1008623226
##########
server/src/main/java/org/apache/iotdb/db/sync/sender/manager/LocalSyncManager.java:
##########
@@ -46,8 +53,30 @@ public LocalSyncManager(DataRegion dataRegion, Pipe pipe) {
/** tsfile */
@Override
- public void syncRealTimeDeletion(Deletion deletion) {
- syncPipe.collectRealTimeDeletion(deletion, dataRegion.getStorageGroupName(), dataRegionId);
+ public void syncRealTimeDeletion(
+ Deletion deletion,
+ List<TsFileResource> unsealedResources,
+ List<TsFileResource> sealedResources) {
+ Set<String> devices = new HashSet<>();
+ unsealedResources.forEach(o -> devices.addAll(o.getDevices()));
+ sealedResources.forEach(o -> devices.addAll(o.getDevices()));
+
+ for (String device : devices) {
+ try {
+ PartialPath path = new PartialPath(device);
+ if (deletion.getPath().overlapWith(path)) {
+ Deletion splitDeletion =
+ new Deletion(path, 0, deletion.getStartTime(), deletion.getEndTime());
Review Comment:
done.
--
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