RongtongJin commented on PR #1250:
URL:
https://github.com/apache/rocketmq-clients/pull/1250#issuecomment-4552446470
I do not think this PR is ready to merge yet. There are several runtime
blockers that need to be fixed before the PHP client can be marked
production-ready:
1. `php/LiteSimpleConsumer.php` builds `ReceiveMessageRequest` with APIs
that do not exist in the generated PHP classes.
Around `LiteSimpleConsumer.php:515`, it calls `setSubscriptions()`,
`setReceiveSystemProperties()`, and `setReceiveTimestamp()`. The generated
`ReceiveMessageRequest` only exposes fields such as `setGroup()`,
`setMessageQueue()`, `setFilterExpression()`, `setBatchSize()`,
`setInvisibleDuration()`, `setLongPollingTimeout()`, and `setAttemptId()`.
Later the receive loop also calls response APIs like `getCase()` /
`getMessages()` that are not present on the generated `ReceiveMessageResponse`,
which has `hasMessage()` / `getMessage()` for a single message. This path will
fatal as soon as `LiteSimpleConsumer::receive()` is used. Please either rewrite
this flow against the actual `ReceiveMessage` proto or update the proto and
regenerate the PHP code consistently.
2. FIFO production does not route by message group.
`Producer::send()` and `sendBatch()` route FIFO messages through the
normal round-robin `takeMessageQueue()` path, even though
`PublishingLoadBalancer::takeMessageQueueByMessageGroup()` exists. This means
messages with the same `messageGroup` can be sent to different queues across
sends, breaking FIFO ordering semantics. FIFO sends should detect
`systemProperties.messageGroup` and select the queue deterministically from
that group.
3. `LiteSimpleConsumer` ignores auth, namespace, and TLS configuration.
The constructor hard-codes `ChannelCredentials::createInsecure()`,
`buildMetadata()` signs with `null` credentials and an empty namespace, and
`LiteSimpleConsumerBuilder::build()` does not pass `tlsCredentials` into the
consumer options. In authenticated, namespaced, or TLS-enabled clusters, this
consumer will not behave like the other client types. It should reuse the same
`ClientTrait` / `RpcClientManager` configuration path as `Producer`,
`PushConsumer`, and `SimpleConsumer`.
4. Composer autoload does not expose the new SDK classes.
`php/composer.json` only registers `GPBMetadata\\` and
`Apache\\Rocketmq\\V2\\`. The newly added SDK classes under
`Apache\\Rocketmq\\` (`ProducerBuilder`, `ClientConfigurationBuilder`,
`MessageBuilder`, `PushConsumer`, etc.) are not autoloadable for users who only
require Composer's `vendor/autoload.php`. The examples work by manually
requiring files, but the package itself will not. Please add the root namespace
mapping, e.g. `"Apache\\Rocketmq\\": ""`, and add at least one test/example
that uses Composer autoload only.
5. `TelemetrySession` is shared too broadly.
`TelemetrySession::getInstance()` keys sessions only by endpoint,
credentials object, and namespace. A producer, simple consumer, and push
consumer in the same process using the same endpoint can therefore share the
same telemetry stream and callbacks. A later client can overwrite settings
callbacks for an earlier client, and closing one client can close the shared
session for another. The session key should include the logical client
identity, at least `clientId` and probably client type.
6. `SubscriptionLoadBalancer` is defined twice.
`php/SimpleConsumerOptimized.php` defines
`Apache\\Rocketmq\\SubscriptionLoadBalancer` at the bottom of the file, while
`php/SubscriptionLoadBalancer.php` defines the same class name separately.
Loading both in one process will fatal with a duplicate class definition, and
the two implementations also differ in queue filtering. Please keep one
implementation and require/import it consistently.
There is also a validation gap: `.github/workflows/php_build.yml` currently
only runs `composer validate` and `composer install`, so the newly added PHP
test files are not exercised in CI. Given the size of this PR and the number of
runtime paths it changes, CI should run the PHP tests before README feature
status is switched from 🚧 to ✅.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
