HIVE-28623: Implement RuntimeContext to decouple runtime data from TezTask#5538
HIVE-28623: Implement RuntimeContext to decouple runtime data from TezTask#5538abstractdog merged 5 commits intoapache:masterfrom
Conversation
|
cc: @tanishq-chugh , @ayushtkn |
abstractdog
force-pushed
the
HIVE-28623
branch
2 times, most recently
from
7fe872c to
2e4cf68
Compare
asf-ci-hive
added
tests passed
tests pending
tests unstable
and removed
tests pending
tests passed
tests unstable
labels
|
|
||
| public void setPlan(QueryPlan plan) { | ||
| this.plan = plan; | ||
| TezTask task = Utilities.getFirstTezTask(plan.getRootTasks()).orElse(null); |
There was a problem hiding this comment.
@abstractdog After query run completes, when resources are being released by the driver, plan is being set to null and it is causing NullPointerException here, which is present as part of DEBUG logs.
There was a problem hiding this comment.
I see, can you point me to the logs that show this?
btw I made a slight refactor in this area after pushing the original, working version that I used in case of the query history...NPE looks weird as we obtain a reference to the RuntimeContext in setPlan...I guess there are calls when the passed plan is already null, need to null-check I guess
There was a problem hiding this comment.
this shouldn't cause any issue wrt query execution but just some noise in logs. a null check wrt plan should just be fine.. i guess the one test failure in precommit is as well linked to this ;)
abstractdog
force-pushed
the
HIVE-28623
branch
from
2e4cf68 to
ecc890e
Compare
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
abstractdog
force-pushed
the
HIVE-28623
branch
from
ecc890e to
ad320a6
Compare
asf-ci-hive
added
tests pending
tests unstable
and removed
tests unstable
tests pending
labels
abstractdog
force-pushed
the
HIVE-28623
branch
from
ad320a6 to
de9f3e0
Compare
| // only set runtimeContext if the plan is not null | ||
| // we don't want to nullify runtimeContext if this method is called with plan=null, which is the case when e.g. | ||
| // driver.releasePlan() tries to release resources/objects that are known to be heavy | ||
| if (plan != null){ |
There was a problem hiding this comment.
nit: missing space between ){
There was a problem hiding this comment.
ack, let me reformat
|
LGTM +1 |
abstractdog
force-pushed
the
HIVE-28623
branch
from
fbebae5 to
c1cde24
Compare
thanks for the review, pushed one more commit to address that |
abstractdog
force-pushed
the
HIVE-28623
branch
from
c1cde24 to
cd39182
Compare
|
thanks @deniskuzZ, addressed comments in cd39182 |
abstractdog
force-pushed
the
HIVE-28623
branch
from
cd39182 to
bef6884
Compare
|
5 participants
What changes were proposed in this pull request?
Encapsulate runtime related info to a separate class for later usage.
Why are the changes needed?
Code refactor.
Does this PR introduce any user-facing change?
No.
Is the change a dependency upgrade?
No.
How was this patch tested?
Easy refactor, precommit tests should pass.