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

Register polling metadata only for polled endpoints #347

Conversation

jlabedo
Copy link
Contributor

@jlabedo jlabedo commented Jul 23, 2024

Description

Every endpoint has some default PollingMetadata registered in the container even if it is not a polled endpoint.
This PR registers PollingMetadata only for polled endpoints.
On a medium sized Symfony repository, I get a 10% speedup on cache:clear.

Type of change

  • Performance

@@ -45,7 +45,7 @@ public function getServiceConfiguration(): ServiceConfiguration
return $this->applicationConfiguration;
}

public function registerPollingEndpoint(string $endpointId, Definition $definition): void
public function registerPollingEndpoint(string $endpointId, Definition $definition, bool $withContinuousPolling = false): void
Copy link
Member

Choose a reason for hiding this comment

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

InboundChannelAdapterBuilder can be set up for cron execution, and continues polling is meant for fixed rate.

However I believe we could simplify this code a bit here, as it's called only for Async Endpoints.
Therefore if we would add fixed rate as a default if PollingMetadata was not passed, then potentially we could remove withContinuousPolling config completely.
Can you check that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean something like this ?

    // packages/Ecotone/src/Messaging/Config/Container/MessagingContainerBuilder.php
    private function getPollingConfigurationForPolledEndpoint(string $endpointId): PollingMetadata
    {
        if (array_key_exists($endpointId, $this->pollingMetadata)) {
            $pollingMetadata = $this->pollingMetadata[$endpointId];
        } else {
            $pollingMetadata = PollingMetadata::create($endpointId)
                ->setFixedRateInMilliseconds(1);
        }

Tests are failing in this case (for e.g. test_serializing_command_and_event_before_sending_to_asynchronous_channel)

Copy link
Member

Choose a reason for hiding this comment

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

Yea I meant something like that.
But maybe in that case, this should be as part of PollingMetadata::create as default configuration if not changed.

Copy link
Member

@dgafka dgafka left a comment

Choose a reason for hiding this comment

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

I've not checked if that's possible, so 🍏.
If we can do it then cool, if not we just continue on the logic we had :)

@jlabedo jlabedo merged commit 2ea57c1 into ecotoneframework:main Jul 26, 2024
30 checks passed
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.

3 participants