TEZ-3363: Delete intermediate data at the vertex level for Shuffle Handler by shameersss1 · Pull Request #60 · apache/tez

shameersss1 · 2020-02-20T10:57:50Z

For applications like pig where processing times can be very long, applications may choose to delete intermediate data for a sub dag. For example if a DAG has synced data to HDFS, all upstream intermediate data can be safely deleted.

shameersss1 · 2022-02-07T11:02:26Z

@abstractdog Could you please review the changes?

abstractdog · 2022-02-07T13:09:31Z

@shameersss1: I'm more than interested in this patch, let me have some time to review it
this needs more thorough testing than TEZ-4129 as TEZ-4129 was on the unhappy code path (failed attempts), but this one seriously affects shuffle
could you please describe what kind of testing process have you done with this patch?

shameersss1 · 2022-02-08T06:39:33Z

@shameersss1: I'm more than interested in this patch, let me have some time to review it this needs more thorough testing than TEZ-4129 as TEZ-4129 was on the unhappy code path (failed attempts), but this one seriously affects shuffle could you please describe what kind of testing process have you done with this patch?

@abstractdog - Thanks for showing interest to review. It has been pending for a while now.

The high level idea behind this feature is that, Whenever all the dependent vertex of a particular vertex have succeeded we delete the vertex shuffle data of that particular/parent vertex.

Testing Procedure

I picked a query which spawns a big dag (preferably some TPC-DS query) which runs to quite some time. I changed number of max reducers to 1 so that the final stage takes time
I checked if the shuffle data of the parent vertex are deleted when all the dependent vertex succeeded.

shameersss1 · 2022-03-02T05:25:27Z

@shameersss1: I'm more than interested in this patch, let me have some time to review it this needs more thorough testing than TEZ-4129 as TEZ-4129 was on the unhappy code path (failed attempts), but this one seriously affects shuffle could you please describe what kind of testing process have you done with this patch?

@abstractdog Could you please review the changes?

abstractdog · 2022-03-02T07:42:26Z

@shameersss1: sorry, I haven't had the chance, I want to test this on a cluster too, where I face some issues at the moment, also I'm busy with other changes, let me get back to you in 2 weeks, thanks for your patience!

abstractdog · 2022-03-10T19:57:24Z

please rebase on top of master, latest compilation error could be due to TEZ-4227

shameersss1 · 2022-03-11T05:48:23Z

please rebase on top of master, latest compilation error could be due to TEZ-4227

Rebased to latest master. pending tests

abstractdog · 2022-03-12T12:51:01Z

+ vertex.appContext.getAppMaster().vertexComplete(
+ vertex.vertexId, nodes);
+ } else {
+ LOG.debug(String.format("The number of incomplete child vertex are %s for the vertex %s",


please use logger format here {} {}, String.format is not necessary

ack. Will resolve in next revision.

abstractdog

left very minor comments, this is very close @shameersss1 !

shameersss1 · 2022-03-13T09:19:22Z

left very minor comments, this is very close @shameersss1 !

I have resolved the comments. Thanks for your valuable feedback.

tez-yetus · 2022-03-13T10:23:44Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 28s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 0s	The patch appears to include 3 new or modified test files.
		_ master Compile Tests _
+0 🆗	mvndep	5m 9s	Maven dependency ordering for branch
+1 💚	mvninstall	9m 18s	master passed
+1 💚	compile	2m 51s	master passed with JDK Ubuntu-11.0.14+9-Ubuntu-0ubuntu2.20.04
+1 💚	compile	2m 41s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	3m 2s	master passed
+1 💚	javadoc	2m 47s	master passed with JDK Ubuntu-11.0.14+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	2m 30s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+0 🆗	spotbugs	0m 42s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	4m 56s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 10s	Maven dependency ordering for patch
+1 💚	mvninstall	1m 47s	the patch passed
+1 💚	compile	1m 44s	the patch passed with JDK Ubuntu-11.0.14+9-Ubuntu-0ubuntu2.20.04
+1 💚	javac	1m 44s	the patch passed
+1 💚	compile	1m 33s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	javac	1m 33s	the patch passed
-0 ⚠️	checkstyle	0m 35s	tez-dag: The patch generated 3 new + 609 unchanged - 1 fixed = 612 total (was 610)
-0 ⚠️	checkstyle	0m 11s	tez-plugins/tez-aux-services: The patch generated 3 new + 67 unchanged - 0 fixed = 70 total (was 67)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	1m 30s	the patch passed with JDK Ubuntu-11.0.14+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	1m 24s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	findbugs	4m 6s	the patch passed
		_ Other Tests _
+1 💚	unit	2m 7s	tez-api in the patch passed.
+1 💚	unit	0m 26s	tez-common in the patch passed.
+1 💚	unit	4m 30s	tez-runtime-library in the patch passed.
+1 💚	unit	4m 28s	tez-dag in the patch passed.
+1 💚	unit	2m 42s	tez-aux-services in the patch passed.
+1 💚	asflicense	0m 48s	The patch does not generate ASF License warnings.
		64m 27s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/7/artifact/out/Dockerfile
GITHUB PR	#60
JIRA Issue	TEZ-3363
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname	Linux dd0e9beab074 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/tez.sh
git revision	master / `132ea4c`
Default Java	Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.14+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
checkstyle	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/7/artifact/out/diff-checkstyle-tez-dag.txt
checkstyle	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/7/artifact/out/diff-checkstyle-tez-plugins_tez-aux-services.txt
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/7/testReport/
Max. process+thread count	2099 (vs. ulimit of 5500)
modules	C: tez-api tez-common tez-runtime-library tez-dag tez-plugins/tez-aux-services U: .
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/7/console
versions	git=2.25.1 maven=3.6.3 findbugs=3.0.1
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

shameersss1 · 2022-03-16T04:33:22Z

@abstractdog - Are we good to take it forward?

abstractdog · 2022-03-16T08:48:06Z

warnings here are easily addressable: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/7/artifact/out/diff-checkstyle-tez-plugins_tez-aux-services.txt
please fix those and then this can be merged

abstractdog · 2022-03-16T08:54:28Z

+ }
+
+ @VisibleForTesting
+ public VertexShuffleDataDeletionContext getVShuffleDeletionContext() {


this is not used currently in any of the tests, vShuffleDeletionContext can remain private to VertexImpl
I'm fine with leaving this part uncovered, can you please remove this method?
(sorry, last minute comments :) )

This is used by testVertexShuffleDelete() in TestVertexImpl

okay, missed it sorry, in this case, you can remove public keyword (make it package visible) to enhance "VisibleForTesting" behavior

ack. I will fix it in next revision.

shameersss1 · 2022-03-16T15:12:32Z

warnings here are easily addressable: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/7/artifact/out/diff-checkstyle-tez-plugins_tez-aux-services.txt please fix those and then this can be merged

ack.

tez-yetus · 2022-03-16T15:40:19Z

💔 -1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 0s	Docker mode activated.
-1 ❌	patch	0m 5s	#60 does not apply to master. Rebase required? Wrong Branch? See https://cwiki.apache.org/confluence/display/TEZ/How+to+Contribute+to+Tez for help.

Subsystem	Report/Notes
GITHUB PR	#60
JIRA Issue	TEZ-3363
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/8/console
versions	git=2.17.1
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

abstractdog · 2022-03-16T15:58:27Z

can you check @shameersss1 if the latest conflict is because of TEZ-4359 and rebase if needed?

…ndler

tez-yetus · 2022-03-16T17:30:44Z

🎊 +1 overall

Vote	Subsystem	Runtime	Comment
+0 🆗	reexec	0m 34s	Docker mode activated.
		_ Prechecks _
+1 💚	dupname	0m 0s	No case conflicting files found.
+1 💚	@author	0m 0s	The patch does not contain any @author tags.
+1 💚	test4tests	0m 1s	The patch appears to include 3 new or modified test files.
		_ master Compile Tests _
+0 🆗	mvndep	5m 11s	Maven dependency ordering for branch
+1 💚	mvninstall	9m 24s	master passed
+1 💚	compile	2m 50s	master passed with JDK Ubuntu-11.0.14+9-Ubuntu-0ubuntu2.20.04
+1 💚	compile	2m 39s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	checkstyle	2m 46s	master passed
+1 💚	javadoc	2m 47s	master passed with JDK Ubuntu-11.0.14+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	2m 32s	master passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+0 🆗	spotbugs	0m 42s	Used deprecated FindBugs config; considering switching to SpotBugs.
+1 💚	findbugs	4m 53s	master passed
		_ Patch Compile Tests _
+0 🆗	mvndep	0m 10s	Maven dependency ordering for patch
+1 💚	mvninstall	1m 47s	the patch passed
+1 💚	compile	1m 45s	the patch passed with JDK Ubuntu-11.0.14+9-Ubuntu-0ubuntu2.20.04
+1 💚	javac	1m 45s	the patch passed
+1 💚	compile	1m 34s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	javac	1m 34s	the patch passed
-0 ⚠️	checkstyle	0m 34s	tez-dag: The patch generated 3 new + 609 unchanged - 1 fixed = 612 total (was 610)
+1 💚	whitespace	0m 0s	The patch has no whitespace issues.
+1 💚	javadoc	1m 30s	the patch passed with JDK Ubuntu-11.0.14+9-Ubuntu-0ubuntu2.20.04
+1 💚	javadoc	1m 26s	the patch passed with JDK Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
+1 💚	findbugs	4m 8s	the patch passed
		_ Other Tests _
+1 💚	unit	2m 6s	tez-api in the patch passed.
+1 💚	unit	0m 27s	tez-common in the patch passed.
+1 💚	unit	4m 35s	tez-runtime-library in the patch passed.
+1 💚	unit	4m 27s	tez-dag in the patch passed.
+1 💚	unit	2m 42s	tez-aux-services in the patch passed.
+1 💚	asflicense	0m 48s	The patch does not generate ASF License warnings.
		64m 28s

Subsystem	Report/Notes
Docker	ClientAPI=1.41 ServerAPI=1.41 base: https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/10/artifact/out/Dockerfile
GITHUB PR	#60
JIRA Issue	TEZ-3363
Optional Tests	dupname asflicense javac javadoc unit spotbugs findbugs checkstyle compile
uname	Linux bbae1f24de65 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9 23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Build tool	maven
Personality	personality/tez.sh
git revision	master / `132ea4c`
Default Java	Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
Multi-JDK versions	/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.14+9-Ubuntu-0ubuntu2.20.04 /usr/lib/jvm/java-8-openjdk-amd64:Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07
checkstyle	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/10/artifact/out/diff-checkstyle-tez-dag.txt
Test Results	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/10/testReport/
Max. process+thread count	2099 (vs. ulimit of 5500)
modules	C: tez-api tez-common tez-runtime-library tez-dag tez-plugins/tez-aux-services U: .
Console output	https://ci-hadoop.apache.org/job/tez-multibranch/job/PR-60/10/console
versions	git=2.25.1 maven=3.6.3 findbugs=3.0.1
Powered by	Apache Yetus 0.12.0 https://yetus.apache.org

This message was automatically generated.

abstractdog

+1

abstractdog · 2022-03-16T18:19:42Z

merged to master, finally! thanks @shameersss1 for your tireless work and patience on this one!

shameersss1 force-pushed the TEZ-3363 branch from d6de416 to a9baac9Compare January 29, 2022 06:33

This comment was marked as outdated.
Sign in to view

shameersss1 force-pushed the TEZ-3363 branch from a9baac9 to 4eb9b0eCompare January 29, 2022 08:52

This comment was marked as outdated.
Sign in to view

abstractdog reviewed Mar 6, 2022
View reviewed changes

Comment threadtez-plugins/tez-aux-services/src/main/java/org/apache/tez/auxservices/ShuffleHandler.java Outdated

abstractdog reviewed Mar 6, 2022
View reviewed changes

Comment threadtez-api/src/main/java/org/apache/tez/dag/api/TezConfiguration.java Outdated

abstractdog reviewed Mar 6, 2022
View reviewed changes

Comment threadtez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/VertexImpl.java

abstractdog reviewed Mar 6, 2022
View reviewed changes

Comment threadtez-plugins/tez-aux-services/src/test/java/org/apache/tez/auxservices/TestShuffleHandler.java Outdated

abstractdog reviewed Mar 6, 2022
View reviewed changes

Comment threadtez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/DAGImpl.java Outdated

abstractdog reviewed Mar 7, 2022
View reviewed changes

Comment threadtez-plugins/tez-aux-services/src/main/java/org/apache/tez/auxservices/ShuffleHandler.java Outdated

abstractdog reviewed Mar 7, 2022
View reviewed changes

Comment threadtez-api/src/main/java/org/apache/tez/dag/api/TezConfiguration.java Outdated

shameersss1 force-pushed the TEZ-3363 branch from 4eb9b0e to c10de16Compare March 10, 2022 15:39

This comment was marked as outdated.
Sign in to view

shameersss1 force-pushed the TEZ-3363 branch from c10de16 to f85d278Compare March 11, 2022 05:47

This comment was marked as outdated.
Sign in to view

shameersss1 force-pushed the TEZ-3363 branch from f85d278 to 0b6908dCompare March 11, 2022 08:50

This comment was marked as outdated.
Sign in to view

shameersss1 requested a review from abstractdog March 11, 2022 09:55

abstractdog reviewed Mar 12, 2022
View reviewed changes

Comment threadtez-api/src/main/java/org/apache/tez/dag/api/TezConfiguration.java Outdated

abstractdog reviewed Mar 12, 2022
View reviewed changes

Comment threadtez-dag/src/main/java/org/apache/tez/dag/app/dag/impl/DAGImpl.java Outdated

abstractdog requested changes Mar 12, 2022
View reviewed changes

shameersss1 force-pushed the TEZ-3363 branch from 0b6908d to 6ea7f14Compare March 13, 2022 09:18

shameersss1 requested a review from abstractdog March 13, 2022 09:19

abstractdog reviewed Mar 16, 2022
View reviewed changes

shameersss1 force-pushed the TEZ-3363 branch 3 times, most recently from 89c05dc to db543c9Compare March 16, 2022 15:35

abstractdog self-requested a review March 16, 2022 15:55

shameersss1 force-pushed the TEZ-3363 branch from db543c9 to c64875aCompare March 16, 2022 16:05

TEZ-3363: Delete intermediate data at the vertex level for Shuffle Ha…
73486cc
…ndler

shameersss1 force-pushed the TEZ-3363 branch from c64875a to 73486ccCompare March 16, 2022 16:15

This comment was marked as outdated.
Sign in to view

abstractdog approved these changes Mar 16, 2022
View reviewed changes

abstractdog merged commit 20873a3 into apache:masterMar 16, 2022

Uh oh!

Conversation

shameersss1 commented Feb 20, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

shameersss1 commented Feb 7, 2022

Uh oh!

abstractdog commented Feb 7, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shameersss1 commented Feb 8, 2022

Uh oh!

shameersss1 commented Mar 2, 2022

Uh oh!

abstractdog commented Mar 2, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

abstractdog commented Mar 10, 2022

Uh oh!

shameersss1 commented Mar 11, 2022

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Uh oh!

abstractdogMar 12, 2022

Choose a reason for hiding this comment

Uh oh!

shameersss1Mar 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abstractdog left a comment

Choose a reason for hiding this comment

Uh oh!

shameersss1 commented Mar 13, 2022

Uh oh!

tez-yetus commented Mar 13, 2022

Uh oh!

shameersss1 commented Mar 16, 2022

Uh oh!

abstractdog commented Mar 16, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abstractdogMar 16, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shameersss1Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

abstractdogMar 16, 2022

Choose a reason for hiding this comment

Uh oh!

shameersss1Mar 16, 2022

Choose a reason for hiding this comment

Uh oh!

shameersss1 commented Mar 16, 2022

Uh oh!

tez-yetus commented Mar 16, 2022

Uh oh!

abstractdog commented Mar 16, 2022

Uh oh!

This comment was marked as outdated.

tez-yetus commented Mar 16, 2022

Uh oh!

abstractdog left a comment

Choose a reason for hiding this comment

Uh oh!

abstractdog commented Mar 16, 2022

Uh oh!

shameersss1 commented Feb 20, 2020•
edited
Loading

abstractdog commented Feb 7, 2022•
edited
Loading

abstractdog commented Mar 2, 2022•
edited
Loading

abstractdog commented Mar 16, 2022•
edited
Loading

abstractdogMar 16, 2022•
edited
Loading