Task.getStatus() and AllocatedPersistentTask.getStatus() can be confusing

[tlrx]

The Task.Status returned by the Task.getStatus() method is overridden and used by persistent tasks to keep their internal state (usually composed of one or more counters for task's progression and a task's specific state). This usage differs from the original purpose of Task.getStatus() which is to provide a quick update of the status of potentially very volatile tasks. The persistent task internal state is also shown in the Task API but I think it should only report progression/stats about the tasks here, while the Cluster State API shows the internal state of persistent tasks as it does today.

We could decorelate AllocatedPersistentTask.getStatus() from Task.Status by introducing a new PersistentTask.State interface and rename AllocatedPersistentTask.getStatus() to something like lastCommitedState().

@martijnvg @bleskes @imotov @polyfractal I'd be happy to have your opinion

[elasticmachine]

Pinging @elastic/es-distributed

[bleskes]

Thanks Tanguy.

used by persistent tasks to keep their internal state. This usage differs from the original purpose of Task.getStatus() which is to provide a quick update of the status of potentially very volatile tasks.

The method is meant imo to expose stats (typically progress). This is still relevant? If people implement this by directly exposing state or creating a dedicated problem isn't something that should concern the framework, in my opinion. Can you explain more as to why do you see this is as different?

[tlrx]

The method is meant imo to expose stats (typically progress).

I completly agree.

This is still relevant? If people implement this by directly exposing state or creating a dedicated problem isn't something that should concern the framework, in my opinion. Can you explain more as to why do you see this is as different?

Some persistent tasks expose a getStats() method in addition to the getStatus() and use getStatus() for persisting a state. My point is that the framework could make this less confusing by letting the persistent tasks expose whatever they want through getStatus() and the Task API (typically stats/progress) while the persistent task state to persist is retrieved through a getState() method.

[bleskes]

Some persistent tasks expose a getStats() method in addition to the getStatus() and use getStatus() for persisting a state.

I see. One more thing I learned today. In that case, should we also rename getStatus() to getStats()? so TM Tasks will report stats and persistent tasks can have a state?

[polyfractal]

My point is that the framework could make this less confusing by letting the persistent tasks expose whatever they want through getStatus() and the Task API (typically stats/progress) while the persistent task state to persist is retrieved through a getState() method.

++ The getStatus()/getState() situation is confusing to someone implementing a persistent task.

Comments on Task#getStatus() seem to suggest they should be used for volatile stats. But then AllocatedPersistentTask#getStatus() overrides this with what looks like state wrapped with a status object, then introduces a new getState() which exposes the state directly and appears to be important to the persistent task framework.

As an implementor of AllocatedPersistentTask it's not clear which -- or if -- you should override? If you override them with your own custom stats/state does that break the allocated task? Are these ever actually persisted somewhere or just for display?

It's also made more confusing since these only apply to the allocated task (I think?), and if the implementor wants to persist some task-specific state that survives task failures, they need to to call persistentTasksService.updateStatus().

@droberts195 might have some thoughts on this as well

[bleskes]

It's also made more confusing since these only apply to the allocated task (I think?), and if the implementor wants to persist some task-specific state that survives task failures, they need to to call persistentTasksService.updateStatus().

Yes - so the idea is not to have getState() but you should indeed periodically call persistentTasksService.updateState (via AllocatedPersistedTask.updatePersistentState) (all using new proposed naming).

[martijnvg]

Double checking: so node tasks will share information via getStats() and if a persistent task needs to store state it should invoke AllocatedPersistedTask.updatePersistentState() (name pending)?

This sounds good to me.

[tlrx]

@martijnvg Yes that's the idea

[jasontedor]

We discussed this in the weekly distributed area meeting and came to agreement on the proposal here.

[tlrx]

Closed in #31031