Skip to content

Feat/embedded amq#200

Merged
brosenberg42 merged 24 commits into
developfrom
feat/embedded-amq
Dec 20, 2023
Merged

Feat/embedded amq#200
brosenberg42 merged 24 commits into
developfrom
feat/embedded-amq

Conversation

@brosenberg42
Copy link
Copy Markdown
Member

@brosenberg42 brosenberg42 commented Dec 8, 2023

Copy link
Copy Markdown
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 16 of 16 files at r1, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @brosenberg42)


docker-compose.core.yml line 43 at r1 (raw file):

          - node.role == manager

  activemq:

The changes you made here also need to be made to thedocker-compose.yml file that we provide as a template to the end users.

Additionally, the entire custom activemq directory needs to be removed. It contains the custom profiles.


components/component-executor.py line 84 at r1 (raw file):

            activemq_host = os.getenv('ACTIVE_MQ_HOST', 'workflow-manager')
            activemq_broker_uri = (
                f'failover:(tcp://{activemq_host}:61616)?maxReconnectAttempts=13&startupMaxReconnectAttempts=21')

Do we want "13" and "21" here? These seem like strange numbers. If so, please add a comment explaining them.


components/component-executor.py line 94 at r1 (raw file):

            os.getenv('WFM_USER', 'admin'),
            os.getenv('WFM_PASSWORD', 'mpfadm'),
            os.getenv('WFM_BASE_URL', 'http://workflow-manager:8080/workflow-manager'),

Please remove the /workflow-manager part.


components/cli_runner/mpf_cli_job_runner.py line 77 at r1 (raw file):

            self._job_props = self._get_combined_job_props(
                cmd_line_args.job_props, env_props, descriptor)
            self._track_type = descriptor['algorithm']['trackType']

Was this missed from the last drop?


markup/docker-entrypoint.sh line 35 at r1 (raw file):

if [ ! "$ACTIVE_MQ_BROKER_URI" ]; then
    export ACTIVE_MQ_BROKER_URI="failover:(tcp://$ACTIVE_MQ_HOST:61616)?maxReconnectAttempts=13&startupMaxReconnectAttempts=21"

Do we want "13" and "21" here? These seem like strange numbers. If so, please add a comment explaining them.


markup/docker-entrypoint.sh line 35 at r1 (raw file):

if [ ! "$ACTIVE_MQ_BROKER_URI" ]; then
    export ACTIVE_MQ_BROKER_URI="failover://(tcp://$ACTIVE_MQ_HOST:61616)?jms.prefetchPolicy.all=0&startupMaxReconnectAttempts=1"

You removed jms.prefetchPolicy.all from all of the broker URIs. According to this, the default value for queues is 1000.

I don't think we want to prefetch since that will mean that one component service can grab more than one job while the other services sit around and do nothing.

Why did you remove the prefetch setting? Is it because it's no longer necessary due to this line in ActiveMQConfiguration.java?

policy.setQueuePrefetch(0);

Copy link
Copy Markdown
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: 14 of 16 files reviewed, 6 unresolved discussions (waiting on @jrobble)


docker-compose.core.yml line 43 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

The changes you made here also need to be made to thedocker-compose.yml file that we provide as a template to the end users.

Additionally, the entire custom activemq directory needs to be removed. It contains the custom profiles.

Done.


components/component-executor.py line 84 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Do we want "13" and "21" here? These seem like strange numbers. If so, please add a comment explaining them.

Done.


components/component-executor.py line 94 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Please remove the /workflow-manager part.

Done.


components/cli_runner/mpf_cli_job_runner.py line 77 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Was this missed from the last drop?

Yes


markup/docker-entrypoint.sh line 35 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

Do we want "13" and "21" here? These seem like strange numbers. If so, please add a comment explaining them.

Done.


markup/docker-entrypoint.sh line 35 at r1 (raw file):

Previously, jrobble (Jeff Robble) wrote…

You removed jms.prefetchPolicy.all from all of the broker URIs. According to this, the default value for queues is 1000.

I don't think we want to prefetch since that will mean that one component service can grab more than one job while the other services sit around and do nothing.

Why did you remove the prefetch setting? Is it because it's no longer necessary due to this line in ActiveMQConfiguration.java?

policy.setQueuePrefetch(0);

It is set on the broker using policy.setQueuePrefetch(0);.

Copy link
Copy Markdown
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 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @brosenberg42)


markup/docker-entrypoint.sh line 35 at r1 (raw file):

if [ ! "$ACTIVE_MQ_BROKER_URI" ]; then
    export ACTIVE_MQ_BROKER_URI="failover://(tcp://$ACTIVE_MQ_HOST:61616)?jms.prefetchPolicy.all=0&startupMaxReconnectAttempts=1"

Double slashes in failover:// are still used in the streaming code.

Copy link
Copy Markdown
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 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @brosenberg42)

@brosenberg42 brosenberg42 merged commit d5dd893 into develop Dec 20, 2023
@brosenberg42 brosenberg42 deleted the feat/embedded-amq branch December 20, 2023 13:44
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.

2 participants