AppResources Controller Refactor with Integration Testing for OpenTSDB

[zack-rma]

Problem Description

Fixes #220.

Solution

Implements integration testing against OpenTSDB.

Builds upon changes in #274

how you tested the change

Includes integration tests.

Where the following done:

• Tests. Check all that apply:
  • Unit tests created or modified that run during ant test.
  • Integration tests created or modified that run during integration testing
    (Formerly called regression tests.)
  • Test procedure descriptions for manual testing
• Was relevant documentation updated?
• Were relevant config element (e.g. XML data) updated as appropriate

If you aren't sure leave unchecked and we will help guide you to want needs changing where.

[zack-rma]

Currently there exists a failing test case due to an unknown EventPort value for the compproc app used in testing. This is causing a connection failure, with the specific location of the issue identified by TODO tasks.

[MikeNeilson]

Not sure if it affects much of the current work but fyi we are planning on removing that event system in favor of standard monitoring tools, likely opentelemtry. Which is not to say you dont have to deal with it just maybe dont put excessive energy in beyond making sure things start. Might be easier to make sure it's more optional at the OpenDCS core level.

…

On Tue, Dec 31, 2024, 7:42 AM Ryan Ripken ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In opendcs-rest-api/src/main/java/org/opendcs/odcsapi/res/AppResources.java <#311 (comment)>: > else if (!cli.isPresent()) { Integer port = appStat.getEventPort(); if (port == null) - return ApiHttpUtil.createResponse(new ArrayList<ApiAppEvent>()); + { + return Response.status(HttpServletResponse.SC_OK) + .entity(new ArrayList<ApiAppEvent>()).build(); + } apiEventClient = new ApiEventClient(appId, appStat.getHostname(), port, appStat.getAppName(), appStat.getPid()); apiEventClient.connect(); - LOGGER.debug("Connected to {}:{}", appStat.getHostname(), port); clientConnectionCache.addApiEventClient(apiEventClient, session.getId()); } else if (appStat.getPid() != null && appStat.getPid() != cli.get().getPid()) { // This means that the app was stopped and restarted since we last checked for events. // Close the old client and open a new one with the correct PID. cli.ifPresent(c -> clientConnectionCache.removeApiEventClient(c, session.getId())); - + Integer port = appStat.getEventPort(); if (port == null) - return ApiHttpUtil.createResponse(new ArrayList<ApiAppEvent>()); // app not running + { + return Response.status(HttpServletResponse.SC_OK) + .entity(new ArrayList<ApiAppEvent>()).build(); + } apiEventClient = new ApiEventClient(appId, appStat.getHostname(), port, appStat.getAppName(), appStat.getPid()); apiEventClient.connect(); This logic is being duplicated. Its unclear to me why the first branch doesn't need to check appStat.getPid() != null but the second branch does. In the second branch you don't need to check cli.ifPresent it has to be present or you would be in the first branch. I think these two could be combined and the code would be clearer. — Reply to this email directly, view it on GitHub <#311 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB44KCHYRAIJ4Q6TWDGJINT2IK3QFAVCNFSM6AAAAABUMWWKKKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDKMRWGU3DCMZYGE> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[rma-rripken]

Resolve my other comments

[zack-rma]

After some discussion, it seems this controller will need to have its functionality verified. It is unclear if this was used in the web UI and whether it was operational before the refactor. There is no current method of obtaining a process ID when starting an app, which is required to obtain a lock in the CP_COMP_PROC_LOCK table. This table is also used to check for running processes and their statuses. We'll likely need to investigate the Toolkit's methods of obtaining a PID and see if it can be used here.

[adamkorynta]

superceded by: #354