Skip to content

Conversation

@losipiuk
Copy link
Member

Description

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

@cla-bot cla-bot bot added the cla-signed label May 28, 2024
@losipiuk losipiuk added no-release-notes This pull request does not require release notes entry and removed cla-signed labels May 28, 2024
@losipiuk losipiuk requested review from dekimir, findepi and wendigo May 28, 2024 07:48
@losipiuk losipiuk force-pushed the lo/spooling-not-found-debug branch from 0f82629 to 7de50e4 Compare May 28, 2024 12:37
@cla-bot cla-bot bot added the cla-signed label May 28, 2024
@losipiuk losipiuk merged commit c4a1110 into trinodb:master May 28, 2024
@losipiuk losipiuk deleted the lo/spooling-not-found-debug branch May 28, 2024 13:25
@github-actions github-actions bot added this to the 449 milestone May 28, 2024
if (updated && newValue.taskStatus().getState().isDone()) {
taskStatusFetcher.updateTaskStatus(newTaskInfo.taskStatus());
finalTaskInfo.compareAndSet(Optional.empty(), Optional.of(newValue));
boolean finalTaskInfoUpdated = finalTaskInfo.compareAndSet(Optional.empty(), Optional.of(newValue));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that we should just refuse to update finalTaskInfo here if missingSpoolingOutputStats is true. Updating it will trigger the listener registered here:

task.addFinalTaskInfoListener(taskInfo -> eventQueue.add(new RemoteTaskCompletedEvent(taskInfo.taskStatus())));

which will eventually trigger this line:
SpoolingOutputStats.Snapshot outputStats = remoteTask.retrieveAndDropSpoolingOutputStats();

and that's forbidden when the spoolingOutputStats is missing.

Storing a null stats here guarantees that the null check will fail in retrieveAndDropSpoolingOutputStats().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed no-release-notes This pull request does not require release notes entry

Development

Successfully merging this pull request may close these issues.

3 participants