Ignition 8 Connector v2 - PR 5 #140

alexjhawk · 2023-04-05T18:04:47Z

I misnumbered the last PR as No.3 when it should've been No.4, so this PR is No.5.

I've updated the previously mentioned PR timeline below:

This PR No. 4 5 - Last significant PR, containing documentation and additional source code changes
Next PR No. 5 6 - Small follow-up pull request to perform code signing and official release.
Next PR No. 5a 6a - Just prior to v2 release (or after, depending on time constraints), a patch should be performed to finalize the Ewon rebranding efforts.
- Most references to the eWon text have been changed to Ewon (proper, updated branding capitalization)
- Some instances of eWon remain, such as the module folder names, which can't easily be changed during an active development cycle (makes Git usage significantly more complex due to files changing also being renamed and/or moved.)
- Add: I think this can be done before the release of v2, and within PR 6. There is not much effort involved, most just testing afterward to ensure compile/run is ok.

alexjhawk · 2023-08-14T14:33:48Z

Also, I imagine it's very possible that this PR will quickly grow in size during the review. To make things more manageable, if something has been resolved to your satisfaction, please feel free to 'Resolve conversation'. This will make the PR much easier to follow and hopefully ensure nothing gets buried or missed.

alexjhawk · 2023-08-14T14:47:36Z

@it-hms @TomKimsey Apologies for another ping.

I meant to include the documentation link originally, but they are visible at https://hms-networks.github.io/IgnitionEwonConnector/.

it-hms · 2023-08-14T19:10:11Z

...ateway/src/main/java/com/hms_networks/americas/sc/ignition/threading/ExceptionUtilities.java

+    logger.error(
+        "Exception Trace Messages: "
+            + Arrays.stream(ExceptionUtils.getThrowables(e))
+                .map(Throwable::getMessage)


message could be null, Not sure if this will be correctly collected and joined

Adding check

it-hms · 2023-08-14T19:19:50Z

...r-gateway/src/main/java/com/hms_networks/americas/sc/ignition/IgnitionEwonConnectorHook.java

+        TagManager.initialize(gatewayContext, connectorSettings);
+      } catch (Exception e) {
+        LOGGER.error(
+            "An error occurred while starting the Ignition Ewon Connector tag manager.", e);


Should the connector continue? Or should it exit here?

Might as well exit -- Adding a startup success flag.

it-hms · 2023-08-14T19:21:16Z

...r-gateway/src/main/java/com/hms_networks/americas/sc/ignition/IgnitionEwonConnectorHook.java

+      dmWebPollingThread =
+          new DMWebPollingThread(
+              dmWebPollingInterval, dmWebPollingIntervalUnit, this, connectorSettings);
+      dmWebPollingThread.start();


Could throw exception executor service has been shutdown. I guess that is very unlikely.

I think this is unnecessary. If the Ignition executor service were shut down, it's very unlikely the module is running/at this point in the code. If it were, I would expect that something would have gone very wrong for the module to still be executing.

it-hms · 2023-08-14T19:38:49Z

...tor-gateway/src/main/java/com/hms_networks/americas/sc/ignition/threading/PollingThread.java

+      gatewayHook
+          .getGatewayContext()
+          .getExecutionManager()
+          .unRegister(IgnitionEwonConnectorHook.BUNDLE_PREFIX_EWON, threadName);


There is also a shutdown method. The limited docs state will gracefully shutdown. Should this be called before unregister?

This is the execution manager of Ignition gateway. Shutting it down would likely have disastrous results.

it-hms · 2023-08-14T19:41:47Z

...ateway/src/main/java/com/hms_networks/americas/sc/ignition/comm/AsyncHttpRequestManager.java

+   *
+   * @since 1.0.0
+   */
+  private static final int CONNECTION_REQUEST_TIMEOUT_SECS = 900;


900 seconds for a connection request? this seems like a really long amount of time. 30 seconds sound more reasonable.

This is not a timeout for the HTTP request/connection itself. It is a timeout where the request can sit in a queue and wait to be performed.

This is necessary to support extremely large Ewon count environments, where, for example, a metadata update can result in hundreds of requests being performed. Due to the throughput limitations of Talk2M/M2Web, all requests cannot be done simultaneously, therefore the connector must have the capacity to queue those requests.

During testing, metadata updates can take a large amount of time. Extending this timeout from its default, which is rather short, significantly reduces the possibility that cascading failures can extend metadata updating much longer.

For example:

Scenario 1: Cxn Request Timeout is Default

Metadata and tag update requests are queued for 200 Ewons at once

50 requests are successful. The remaining 150 fail because of a cxn request timeout

Due to throughput limitations, the requests simply could not have been started sooner and timed out before they could even be attempted

Scenario 2: Cxn Request Timeout is Increased (900)

Metadata and tag update requests are queue for 200 Ewons at once

200 requests are successful. None fail due to the increased cxn request timeout

Should this be renamed to provided a better description of its functionality

Would you happen to have any suggestions?

I hoped the comment provided context because these timeout variable names align with their counterpart names in the HTTP library itself.

Would CONNECTION_REQUEST_QUEUE_TIMEOUT_SECS work?

Yes. Will update

it-hms · 2023-08-14T19:45:49Z

...ateway/src/main/java/com/hms_networks/americas/sc/ignition/comm/AsyncHttpRequestManager.java

+      // Create connection manager
+      try {
+        asyncClientConnectionManager = new PoolingAsyncClientConnectionManager();
+        asyncClientConnectionManager.setMaxTotal(HTTP_CXN_POOL_MAX_REQUESTS);


This seems like a lot. Could be memory intensive.

This is necessary to buffer requests and, similar to the Cxn request timeout, is large enough to support accounts with many Ewons.

The memory intensity would be related/tied to the number of Ewons on the account, which influences the number of necessary requests.

it-hms · 2023-08-14T19:47:11Z

...ateway/src/main/java/com/hms_networks/americas/sc/ignition/comm/AsyncHttpRequestManager.java

+                .build();
+        httpAsyncClient.start();
+      } catch (Exception e) {
+        LOGGER.error("Failed to create HTTP connection manager.", e);


Should application continue if there is an error here?

Might as well exit -- Adding a return value and using it to update the startup success flag

it-hms · 2023-08-14T19:55:58Z

...eway/src/main/java/com/hms_networks/americas/sc/ignition/comm/M2WebCommunicationManager.java

+   */
+  public static Future<M2WebGetEwonsResponse> getEwonGatewayList(
+      CommunicationAuthInfo communicationAuthInfo) {
+    FutureCallback<SimpleHttpResponse> httpResponseFutureCallback = null;


Is this used? the callback is null?

This method stub is used.

Several additional method stubs were implemented because a large portion of the M2Web/DMWeb API code was designed to be close to/similar to a library, allowing us to reuse code in the future easily.

The callback is null because we are waiting on the future (non-async request), and the callback is unused by the calling code.

it-hms · 2023-08-14T19:57:20Z

I did not get a chance to fully review. I did not see any errors or anything that would require changes, but I did have some questions about should the code exit in some places.

Adds the ExceptionUtilities class which provides useful functionality for working with exceptions, including printing of the message text for an entire exception trace.

Replaced the existing GatewayHook.java class with the new IgnitionEwonConnectorHook.java class which has been rewritten to support asynchronous HTTP requests and multithreaded response handling.

Adding this gateway hook variable to the polling thread class allows for threads to trigger a connector shutdown in the event of an unknown critical error.

Improve the polling thread class by migrating it to use the internal Ignition execution manager and adding an assigned name.

Improve the performance of the AsyncHttpRequestManager class by explicitly defining a number of parameters, including response timeout, connection request timeout, IO thread count, and increased route configuration.

TomKimsey · 2023-08-17T15:23:04Z

Still reviewing, this is not forgotten.

TomKimsey

Please standardize on the number of blank newlines at the end of md/x files. It should either be 0 or 1 for consistency.

...rc/main/java/com/hms_networks/americas/sc/ignition/threading/M2WebMetadataPollingThread.java

eWonConnector-build/pom.xml

web-docs/docs/_partial/_sys_req_hardware.mdx

web-docs/docs/05-usage/03-ALLOWED_TAG_NAMES.mdx

TomKimsey · 2023-08-18T11:33:20Z

web-docs/docs/_partial/_sys_req_hardware.mdx

+The minimum hardware requirements are suitable for a small number of devices and tags. If you are planning to connect
+many devices or tags, you should consider increasing the hardware resources available to Ignition and its modules.
+
+These requirements match those of Ignition, with additional memory (RAM) required for the connector to run.


How was this determined

There is no advanced determination here. It is simply the Ignition requirements with additional memory to support the connector. The connector will use at least some memory, but the amount of tags/Ewons really controls the amount.

The idea of adding a minimum beyond what the Ignition defaults are is that 'guarantees' that the connector has its own dedicated resources of some amount. It also brings to attention that system resources, including memory, must be considered, as mentioned in the larger comment for the recommended hardware.

In many cases, the minimum hardware requirements may be adequate (for example, my developer/testing account with only a handful of Ewons + date has no issue with 4 GB). The minimum requirement of 4 GB can still cause concern on large customer systems, though, where hundreds of tags and Ewons are in place. Upping to 8 GB, in that case, may improve the connector performance and reduce the change of Ignition time/clock drift due to the connector overloading the system.

TomKimsey · 2023-08-18T11:34:29Z

web-docs/docs/_partial/_sys_req_hardware.mdx

+resources available to Ignition will limit the number of devices and tags that can be connected. If you experience
+a degradation in performance, it is recommended to increase the hardware resources available to Ignition and its


Should you mention network as a source of performance degradation?

Added a small message that network bandwidth could be increased.

I think this probably should get rephrased. The way it reads right now is if you have any performance issue you should get a better cpu, increase ram, increase network bandwidth, and increase hdd space.

a protentional issue could read
Title
"Rephrase performance degradation section in web-docs/docs/_partial/_sys_req_hardware.mdx"
Msg
"Rephrase section to add some protentional troubleshooting steps to identify and resolve the bottleneck"

You might even rephrase to say that decreased or sub optimal performance could be due to one or more of the following: cpu availability/speed, memory, network, disk space, etc.

Issue #145 was created

web-docs/docs/05-usage/03-ALLOWED_TAG_NAMES.mdx

web-docs/docs/03-QUICK_START_GUIDE.mdx

Fixed a bug with the Ewon-specific realtime override property which allowed it to be configured once, but not be detected or loaded when the connector restarts or boots.

Updated the javadoc for #DEFAULT_LIVE_POLL_RATE in the EwonConnectorSettings class to use the correct time unit of minutes instead of seconds.

Add a success/failure return tag to the initialize method in AsyncHttpRequestManager.java so that the connector start up can be aborted if there is a failure.

Improve the branding throughout the connector by more consistently referencing the connector name as Ignition Ewon Connector. Additionally, eWon is being corrected to Ewon where possible. The Ignition Maven Plugin was also updated to support improved build version display on the Ignition UI. Note: Further branding improvements will be completed after v2.0.0 release to alleviate the need for multiple rebases during dev.

Migrated existing documentation to the new Solution Center Docusaurus system, including reorganization and setup. Additionally, the documentation was updated or added to in order to reflect changes made in the v2.0.0 release.

TomKimsey · 2023-08-18T18:08:36Z

web-docs/docs/_partial/_sys_req_hardware.mdx

+resources available to Ignition will limit the number of devices and tags that can be connected. If you experience
+a degradation in performance, it is recommended to increase the hardware resources available to Ignition and its


I think this probably should get rephrased. The way it reads right now is if you have any performance issue you should get a better cpu, increase ram, increase network bandwidth, and increase hdd space.

a protentional issue could read
Title
"Rephrase performance degradation section in web-docs/docs/_partial/_sys_req_hardware.mdx"
Msg
"Rephrase section to add some protentional troubleshooting steps to identify and resolve the bottleneck"

alexjhawk self-assigned this Apr 5, 2023

alexjhawk force-pushed the Ignition-8-alha branch 19 times, most recently from 0cd23bb to 70652fd Compare April 12, 2023 15:42

alexjhawk force-pushed the Ignition-8-alha branch 10 times, most recently from 45b59ae to 474f053 Compare June 2, 2023 17:32

it-hms reviewed Aug 14, 2023

View reviewed changes

Add ExceptionUtilities.java

c165ca0

Adds the ExceptionUtilities class which provides useful functionality for working with exceptions, including printing of the message text for an entire exception trace.

alexjhawk force-pushed the Ignition-8-alha branch from 56962d2 to 24a3d64 Compare August 14, 2023 20:34

alexjhawk requested a review from it-hms August 14, 2023 20:34

alexjhawk force-pushed the Ignition-8-alha branch from 24a3d64 to b45c1ee Compare August 14, 2023 20:38

alexjhawk and others added 6 commits August 14, 2023 16:38

Replace GatewayHook class

67d7be7

Replaced the existing GatewayHook.java class with the new IgnitionEwonConnectorHook.java class which has been rewritten to support asynchronous HTTP requests and multithreaded response handling.

Add gateway hook var to PollingThread

99a1094

Adding this gateway hook variable to the polling thread class allows for threads to trigger a connector shutdown in the event of an unknown critical error.

Improve PollingThread

6fae000

Improve the polling thread class by migrating it to use the internal Ignition execution manager and adding an assigned name.

Improve AsyncHttpRequestManager.java

4b52a71

Improve the performance of the AsyncHttpRequestManager class by explicitly defining a number of parameters, including response timeout, connection request timeout, IO thread count, and increased route configuration.

Add DMWeb communication mgr

0457959

Add M2Web communication mgr

734f955

alexjhawk force-pushed the Ignition-8-alha branch from b45c1ee to 1a5b999 Compare August 15, 2023 13:45

Add DMWeb polling thread

45f7d90

TomKimsey requested changes Aug 18, 2023

View reviewed changes

alexjhawk force-pushed the Ignition-8-alha branch from 1a5b999 to 36406c0 Compare August 18, 2023 12:57

alexjhawk added 7 commits August 18, 2023 08:57

Add M2Web metadata polling thread

e027813

Add M2Web polling thread

8f0c95a

Fix TagManager realtime bug

1a6be71

Fixed a bug with the Ewon-specific realtime override property which allowed it to be configured once, but not be detected or loaded when the connector restarts or boots.

Update EwonConnectorSettings.java

974bcd9

Updated the javadoc for #DEFAULT_LIVE_POLL_RATE in the EwonConnectorSettings class to use the correct time unit of minutes instead of seconds.

Add HTTP mgr initialize success flag

7f60a62

Add a success/failure return tag to the initialize method in AsyncHttpRequestManager.java so that the connector start up can be aborted if there is a failure.

Add improved documentation

36406c0

Migrated existing documentation to the new Solution Center Docusaurus system, including reorganization and setup. Additionally, the documentation was updated or added to in order to reflect changes made in the v2.0.0 release.

alexjhawk requested a review from TomKimsey August 18, 2023 13:00

TomKimsey approved these changes Aug 18, 2023

View reviewed changes

it-hms approved these changes Aug 21, 2023

View reviewed changes

alexjhawk merged commit 07649c5 into main Aug 21, 2023

		resources available to Ignition will limit the number of devices and tags that can be connected. If you experience
		a degradation in performance, it is recommended to increase the hardware resources available to Ignition and its

Ignition 8 Connector v2 - PR 5 #140

Ignition 8 Connector v2 - PR 5 #140

Conversation

alexjhawk commented Apr 5, 2023 • edited Loading

alexjhawk commented Aug 14, 2023

alexjhawk commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

it-hms commented Aug 14, 2023

TomKimsey commented Aug 17, 2023

TomKimsey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexjhawk commented Apr 5, 2023 •

edited

Loading