-
Notifications
You must be signed in to change notification settings - Fork 323
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Keep single execute expression job in the queue #9077
Keep single execute expression job in the queue #9077
Conversation
Tested manually. A fully async test would introduce a race without being able to synchronize the test with the program execution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am surprised there can only be at most one oneshotVisualization
. That doesn't seem correct to me, but I don't see all the nuances of the implementation.
|
||
@Override | ||
public boolean equalsTo(UniqueJob<?> that) { | ||
return that instanceof ExecuteExpressionJob; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Equality based on class of the job? E.g. only one job may be present regardless of the actual oneShotExpression
?
It seems to me that the engine is trying to handle cancellation without understanding what is being cancelled. I think only the IDE can say that a oneShotExpression
and its ExecuteExpressionJob
shall be cancelled - the engine cannot decide on that.
): Unit = | ||
synchronized { | ||
val state = contexts(contextId) | ||
state.visualizations.setOneshotExpression(oneshotExpression) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can there be only one oneshotExpression
? That seems suspicious to me.
Especially when (one shot) executions requests are co-leased the oneshotExpression
s can accumulate - one the (last) execution finishes, all the oneshotExpression
s need to be evaluated and the results delivered to the IDE, I think.
As discussed on the call. One expression per execution context should work in the current use case |
Pull Request Description
close #8965
Changelog:
ExecuteExpressionJob
in the queueExecuteExpressionCommand
synchronous to preserve the order of commandsVisualization
andOneshotExpression
to simplify the logicImportant Notes
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
./run ide build
.