Skip to content
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 #2814

Merged
merged 37 commits into from
Jan 9, 2025

Conversation

bncriju
Copy link
Contributor

@bncriju bncriju commented Dec 16, 2024

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

Please note that earlier pull request got auto closed while renamed the branch name to match the correct kie-issue and the comments in the previous pull request are already incorporated in this one.

@bncriju bncriju requested a review from tiagobento as a code owner December 16, 2024 12:57
@yesamer yesamer requested a review from pefernan December 16, 2024 13:45
@yesamer yesamer requested a review from danielzhe December 17, 2024 08:55
Copy link
Contributor

@pefernan pefernan left a comment

Choose a reason for hiding this comment

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

@bncriju great work! I cannot get the counts initialized to 0 the first time I open Dev UI for first time (they stay as -). Would you mind taking a look?

case "JobEvent":
maybeRun(onJobEvent);
break;
default:
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can get rid of the default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

public void refresh() {
vertx.setTimer(500, id -> {
Copy link
Contributor

Choose a reason for hiding this comment

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

This timer looks like to short, in slow laptops (like main) this may not work, but I think we can go with it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and adjusted to 1000 instead of 500

@bncriju
Copy link
Contributor Author

bncriju commented Dec 19, 2024

Thanks for the comments @pefernan. Still cannot get the counts initialized to 0 the first time I open Dev UI for first time (they stay as -), but this works fine as expected when we open process-compact-architecture.

@bncriju bncriju requested a review from pefernan December 19, 2024 19:00
@bncriju
Copy link
Contributor Author

bncriju commented Jan 8, 2025

Done all the changes, and could you have a review once again

Copy link
Contributor

@pefernan pefernan left a comment

Choose a reason for hiding this comment

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

@bncriju great work, I'd just clean up a bit the logs to avoid printing info on every count refresh and that would be all.

}

private void refreshCount() {
LOGGER.info("Refreshing data for query: {}", query);
Copy link
Contributor

Choose a reason for hiding this comment

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

@bncriju I'd change this to debug to have the logs a bit more clean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@danielzhe
Copy link
Contributor

Good job, @bncriju !

I just found some minor issues in the jbpm-dev-ui:

  1. The Process Instances, Tasks and Jobs does not automatically update in the first run. I mean, if we have zero process and start a new one, the count just start if I refresh the page (pressing F5). After that, they update automatically without the need of refresh.

  2. It looks like the counters doesn't decrement. I started 3 process, waited them to be finished and the counters still at 3. For the process instances it may be correct, since they are completed, but I'm not sure about the tasks. Even applying the filters, I can't see any task:
    image

I don't know if this is an issue, but I think it doesn't looks correct. It can be an issue related to filters in tasks either. I'm not sure. What do you think @pefernan ?

@bncriju bncriju requested a review from pefernan January 9, 2025 07:56
@pefernan
Copy link
Contributor

pefernan commented Jan 9, 2025

@danielzhe yes, issue 1 is something I already found. Looks like the new subscriptions added to the counter Multi aren't updated with the new values.. I provided @bncriju a PR with a fix (bncriju#2)

Regarding 2.. yes, that's a filter thing, we are currently showing the count of all elements (process instances, tasks or jobs) indexed in data-index. When we first created we wanted to show a snapshot of what was present on the system, but we can definitelly change it. In the end I think that it's just matter of deciding what we want to display there, I don't have a strong opinion.
IMO It would be cool to have different counters for each label and show different data, but at this point I think Quarkus doesn't allow that, maybe in future versions.

- Adding `MultiCacheOp` to automatically replying emitted data to new subscribers
Copy link
Contributor

@pefernan pefernan left a comment

Choose a reason for hiding this comment

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

great job @bncriju! I think we have it :D

@bncriju
Copy link
Contributor Author

bncriju commented Jan 9, 2025

thanks Pere and Daniel for all the inputs and suggestions on this. now, in good position to merge seeing the ci build status.

@bncriju bncriju requested a review from pefernan January 9, 2025 11:37
Copy link
Contributor

@danielzhe danielzhe left a comment

Choose a reason for hiding this comment

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

Great job!

@yesamer
Copy link
Contributor

yesamer commented Jan 9, 2025

Merging, the failing 'CI :: License headers' is unrelated and already fixed.

@yesamer yesamer merged commit 0832d4e into apache:main Jan 9, 2025
14 of 15 checks passed
@bncriju bncriju deleted the kie-issues#1669 branch January 9, 2025 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jBPM Quarkus DevUI seems to not update the # of items
7 participants