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

Feat/angular jobs2 #1627

Merged
merged 11 commits into from Mar 20, 2023
Merged

Feat/angular jobs2 #1627

merged 11 commits into from Mar 20, 2023

Conversation

brosenberg42
Copy link
Member

@brosenberg42 brosenberg42 commented Dec 12, 2022

@brosenberg42 brosenberg42 self-assigned this Dec 12, 2022
@brosenberg42 brosenberg42 mentioned this pull request Dec 12, 2022
@brosenberg42
Copy link
Member Author

trunk/workflow-manager/src/test/java/org/mitre/mpf/wfm/util/TestPropertiesUtil.java line 59 at r1 (raw file):

public class TestPropertiesUtil {

This test, PropertiesUtil, and MpfPropertiesConfigurationBuilder all had to be changed because when these tests failed it caused other tests to fail. I initially saw the issue on Jenkins. I didn't update one of the tests correctly so it failed, but then system tests also failed. When debugging the tests locally, after the tests failed once, they would start failing at a different part. That was because the tests modify mpf-custom.properties.

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 434 of 437 files at r1, 3 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @brosenberg42)

a discussion (no related file):
The {} button on the Job Status page shows a blank page. It's not displaying the JSON.

The fix is how I added "lib" to this line in output_object.jsp:

<script src="../resources/js/lib/jquery-1.11.0.min.js"></script>

a discussion (no related file):
The "TiesDb" and "Callback" IN PROGRESS column spinners continue to spin even when the job is complete. I need to refresh the page for the table cells to show the expected "ERROR" button.

This happens in FF and Chrome when I have broadcasts enabled. It does not happen when I have polling enabled since the page refreshes on the next poll.



trunk/workflow-manager/src/main/java/org/mitre/mpf/Application.java line 124 at r2 (raw file):

        servlet.addInitParameter(
            "org.atmosphere.interceptor.HeartbeatInterceptor.heartbeatFrequencyInSeconds", "60");

What impact does this have? Is this just to keep the atmosphere web socket open?


trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/data/access/hibernate/HibernateJobRequestDaoImpl.java line 98 at r3 (raw file):

    @Override
    public void newJobCreated() {
        _jobCount.incrementAndGet();

I noticed we're not doing _startupCountFuture.join(); here. Is that because the countAll thread was already kicked off such that any new jobs added to the DB since then won't be included in that initial query results?


trunk/workflow-manager/src/main/webapp/resources/ui-plugins/datatables-plugins/datatables-responsive/Readme.md line 0 at r3 (raw file):
Is this file supposed to be empty?


trunk/workflow-manager/src/main/webapp/resources/layouts/directives/navbar.html line 46 at r3 (raw file):

                    <a class="dropdown-toggle active" id="jobs_dropdown" data-toggle="dropdown" role="button" aria-haspopup="true" aria-expanded="false">Jobs<span class="caret"></span></a>
                    <ul class="dropdown-menu">
                        <li><a ui-sref="/jobs" id="menu_jobs" title="View status of queued jobs">Status</a></li>

I had to change this to ui-sref="jobs" so that I could use the "Job" --> "Status" link in the top menu bar.

Copy link
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @jrobble)


trunk/workflow-manager/src/main/java/org/mitre/mpf/Application.java line 124 at r2 (raw file):

Previously, jrobble (Jeff Robble) wrote…

What impact does this have? Is this just to keep the atmosphere web socket open?

This doesn't affect how long the web socket stays open. I increased it because the heartbeat messages were producing a lot of unnecessary network activity.


trunk/workflow-manager/src/main/java/org/mitre/mpf/wfm/data/access/hibernate/HibernateJobRequestDaoImpl.java line 98 at r3 (raw file):

Previously, jrobble (Jeff Robble) wrote…

I noticed we're not doing _startupCountFuture.join(); here. Is that because the countAll thread was already kicked off such that any new jobs added to the DB since then won't be included in that initial query results?

We don't need to know how many jobs are in the database to create a new job, so there is no reason to call _startupCountFuture.join(). The thing you said about count all is also accurate, that is why we use _jobCount.addAndGet(countAll(session)); instead of _jobCount.set(countAll(session));


trunk/workflow-manager/src/main/webapp/resources/ui-plugins/datatables-plugins/datatables-responsive/Readme.md line at r3 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Is this file supposed to be empty?

I don't know why it is empty. All I did was move stuff to a different directory.

@jrobble
Copy link
Member

jrobble commented Mar 10, 2023

trunk/workflow-manager/src/main/webapp/resources/js/directives.js line 368 at r3 (raw file):

            // uib-pagination does not disable links with ellipses.
            const disableEllipses = () => {

We discussed changing the "Next" text in the rightmost pagination button to "More" when doing a search. One way to do that would to modify the link text inside the pagination-next element. Another easier way may be to set the uib-pagination next-text setting.

Copy link
Member Author

@brosenberg42 brosenberg42 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 433 of 438 files reviewed, 7 unresolved discussions (waiting on @brosenberg42 and @jrobble)

a discussion (no related file):

Previously, jrobble (Jeff Robble) wrote…

The {} button on the Job Status page shows a blank page. It's not displaying the JSON.

The fix is how I added "lib" to this line in output_object.jsp:

<script src="../resources/js/lib/jquery-1.11.0.min.js"></script>

Done.


a discussion (no related file):

Previously, jrobble (Jeff Robble) wrote…

The "TiesDb" and "Callback" IN PROGRESS column spinners continue to spin even when the job is complete. I need to refresh the page for the table cells to show the expected "ERROR" button.

This happens in FF and Chrome when I have broadcasts enabled. It does not happen when I have polling enabled since the page refreshes on the next poll.

Done.



trunk/workflow-manager/src/main/webapp/resources/js/directives.js line 368 at r3 (raw file):

Previously, jrobble (Jeff Robble) wrote…

We discussed changing the "Next" text in the rightmost pagination button to "More" when doing a search. One way to do that would to modify the link text inside the pagination-next element. Another easier way may be to set the uib-pagination next-text setting.

Done.


trunk/workflow-manager/src/main/webapp/resources/layouts/directives/navbar.html line 46 at r3 (raw file):

Previously, jrobble (Jeff Robble) wrote…

I had to change this to ui-sref="jobs" so that I could use the "Job" --> "Status" link in the top menu bar.

Done.

Copy link
Member

@jrobble jrobble left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r4, 5 of 7 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)

@brosenberg42 brosenberg42 merged commit 482494b into develop Mar 20, 2023
@brosenberg42 brosenberg42 deleted the feat/angular-jobs2 branch March 20, 2023 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants