-
Notifications
You must be signed in to change notification settings - Fork 188
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
kie-issues#1669: jBPM Quarkus DevUI seems to not update the # of items #2781
Conversation
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.
Thank you for the PR, I have some points for a discussion before merge.
LOGGER.warn("Query: " + graphModelName); | ||
doQuery(query, graphModelName).toCompletionStage() | ||
.thenAccept(result -> { | ||
LOGGER.warn("Query: " + graphModelName + ". Received response: " + result); | ||
stream.onNext(result); |
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.
is the warning
level needed here? sounds like good event for info
or debug
, but I am not sure, maybe there is good reason to have warn
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.
yea I'd remove this to avoid verbosity. Only log if there's an error on the query.
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.
Done. Incorporated in new PR
} | ||
|
||
private void maybeRun(Runnable runnable) { | ||
if(Objects.nonNull(runnable)) { |
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.
@tiagobento hi, does Prettier
format only non java files? I would expect if () {
instead of if() {
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.
Done. Incorporated in new PR
import java.util.Collection; | ||
import java.util.Objects; | ||
|
||
@ApplicationScoped |
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.
Please add the @IfBuildProfile("dev") here, to make sure that this bean is only available in dev mode!
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.
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.
This broke the build, @pefernan . Maybe more changes need to be made or it need to have a prod bean? I don't know.
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.
Done. Incorporated in new PR
@@ -41,6 +44,10 @@ | |||
public class JBPMDevuiJsonRPCService { | |||
private static final String DATA_INDEX_URL = "kogito.data-index.url"; | |||
|
|||
private final BroadcastProcessor<String> processesStream = BroadcastProcessor.create(); |
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 think we said to move this to Multi
, since the BroadcastProcessor
don't cache values, or have an intermediate bean that acts as a values producer for the Multi
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.
Done. Incorporated in new PR
I fixed the broken build locally and published here: danielzhe@329e23e The fix that I made it was just to make it buildable, I don't know if the code this way is your intention, because of the use of |
private Multi<String> multi; | ||
private WebClient dataIndexWebClient; | ||
|
||
public DataIndexCounter(String query, String graphname, BroadcastProcessor<String> stream,WebClient dataIndexWebClient, JBPMDevUIEventPublisher eventPublisher) { |
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.
The JBPMDevUIEventPublisher eventPublisher
is not being used anywhere here.
Also, you can format the parameters with a space between each one:
String query, String graphname, BroadcastProcessor<String> stream, WebClient dataIndexWebClient
(the stream,WebClient
are merged)
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.
Done. Incorporated in new PR
private static final Logger LOGGER = LoggerFactory.getLogger(DataIndexCounter.class); | ||
|
||
private Multi<String> multi; | ||
private WebClient dataIndexWebClient; |
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.
You can set those fields as final
, since their instances are not changed.
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.
Done. Incorporated in new PR
Thanks for comments on this PR. All of those are incorporated in new PR #2814 after renaming of the branch to match the right kie-issue |
The wrong behavior is rectified by making the Process Instances, Tasks and Jobs to get dynamically updated, as and when user defines a process instance.
Closes apache/incubator-kie-issues#1669