Harmonize reflector topics between mqtt and gbos providers#1278
Harmonize reflector topics between mqtt and gbos providers#1278noursaidi wants to merge 9 commits into
mqtt and gbos providers#1278Conversation
The `IotReflectorClient` used a provider-specific check for the `MQTT` provider, forcing it to use a flat `reflect` topic for both subscribing and publishing. This was inconsistent with the `GBOS` provider and other UDMI-standard topic structures. This change harmonizes the behavior by ensuring that all providers use: - `state` for reflector state updates (reflector topic) - `events/udmi` for published messages (publish topic) Modified `validator/src/main/java/com/google/bos/iot/core/proxy/IotReflectorClient.java` to return `STATE_TOPIC` and `UDMI_TOPIC` directly in `getReflectorTopic()` and `getPublishTopic()`, removing the special-case logic for `IotProvider.MQTT`. Co-authored-by: noursaidi <9341216+noursaidi@users.noreply.github.com>
This change standardizes the MQTT topics used by the reflector tool to match the UDMI standard, aligning 'mqtt' provider behavior with 'gbos'. Key changes: 1. **Validator**: Modified `IotReflectorClient.java` to use standard `state` and `events/udmi` topics for all providers, removing the special-case `reflect` topic for MQTT. 2. **UDMIS Messaging**: Enhanced `SimpleMqttPipe.java` to support absolute MQTT topic subscriptions (starting with `/`) and multiple comma-separated topics in `recv_id`. 3. **Configuration**: Updated `udmis/etc/local_pod.json` to subscribe the `reflect` flow to both the legacy `reflect` topic and the new harmonized absolute topic pattern `/r/UDMI-REFLECT/d/+/#`. This alignment ensures consistent behavior across transport providers and resolves CI timeouts caused by topic mismatches between the tool and the service. Co-authored-by: noursaidi <9341216+noursaidi@users.noreply.github.com>
Standardized the MQTT topics used by the reflector tool to match the UDMI standard, aligning 'mqtt' provider behavior with 'gbos'.
Key changes:
1. **Validator**: Modified `IotReflectorClient.java` to use standard `state` and `events/udmi` topics for all providers, removing the special-case `reflect` topic for MQTT.
2. **UDMIS Messaging**: Enhanced `SimpleMqttPipe.java` to support absolute MQTT topic subscriptions (starting with `/`) and multiple comma-separated topics in `recv_id`.
3. **Configuration**: Updated `udmis/etc/local_pod.json` to subscribe the `reflect` flow to the legacy `reflect` topic and the new harmonized absolute topic patterns `/r/UDMI-REFLECT/d/+/state` and `/r/UDMI-REFLECT/d/+/events/#`.
4. **Stability Fixes**:
- Added a null check in `IotReflectorClient.java` for `events.logentries` to prevent NPEs.
- Ensured `updateRegistry` explicitly sets `resource_type` to `REGISTRY` in `IotReflectorClient.java`.
- Increased the reflector configuration synchronization timeout to 30 seconds to mitigate CI flakes.
This alignment ensures consistent behavior across transport providers and improves system stability.
Co-authored-by: noursaidi <9341216+noursaidi@users.noreply.github.com>
…-topics-mqtt-gbos-18006755468579886937
This reverts commit 5186488.
Standardized the MQTT topics used by the reflector tool to match the UDMI standard, aligning 'mqtt' (and 'implicit') provider behavior with 'gbos'. Key changes: 1. **Validator**: Modified `IotReflectorClient.java` to use standard `state` and `events/udmi` topics for all providers, removing special-case `reflect` topic logic. 2. **Robustness**: Added null checks for `logentries` and `reflectorConfig` in `IotReflectorClient.java`. 3. **Routing**: Fixed `updateRegistry` to explicitly set `resource_type` to `REGISTRY` for correct routing in the pipeline. 4. **UDMIS Messaging**: Enhanced `SimpleMqttPipe.java` to support absolute topics and multiple subscriptions. 5. **Configuration**: Updated `local_pod.json` to maintain backward compatibility while supporting new UDMI-standard topics. 6. **CI Reliability**: Increased handshake and config sync timeouts in `MqttPublisher.java` and `IotReflectorClient.java`. 7. **Implicit Provider**: Updated `MqttPublisher.java` to treat `IMPLICIT` identically to `MQTT`. This unification ensures consistent behavior across all transport providers. Co-authored-by: noursaidi <9341216+noursaidi@users.noreply.github.com>
grafnu
left a comment
grafnu
left a comment
There was a problem hiding this comment.
The reason the topic structure was different was somewhat just so that we knew it could be different in the MQTT space. Not sure if it's better to be dynamic and flexible, or strict and pedantic. Both have their uses. But, bottom line is I'm not sure it matters for now and honestly I'm entirely sure what you're referring to, specifically, since it still seems like there's two fundamentally different topic structures, but that's ok.
| private static final String CLOUD_IOT_CONFIG_JSON = "cloud_iot_config.json"; | ||
| private static final Pattern SPEC_PATTERN = Pattern.compile( | ||
| "(//([a-z]+)/)?(([a-z-]+))(/([a-z0-9]+))?(\\+([a-z0-9-]+))?"); | ||
| "(//([a-z]+)/)?(([a-z-\\.]+))(/([a-z0-9]+))?(\\+([a-z0-9-]+))?"); |
There was a problem hiding this comment.
still confused as to what this is for... did this sneak in again or is this actually used/tested somewhere?
There was a problem hiding this comment.
I accidently comitted PR #1243 to main. I reverted it in this branch, but Jules added it back again ...
| "flows": { | ||
| "reflect": { | ||
| "recv_id": "reflect", | ||
| "recv_id": "reflect,/r/UDMI-REFLECT/d/+/state,/r/UDMI-REFLECT/d/+/events/#", |
There was a problem hiding this comment.
This... just got ugly. The problem with this kind of overloading is that it's very cryptic (even more cryptic than arbitrarily named fields with no semantic meaning). What's the intent here, is it just to be more focused on what it subscribes too (so it's not everything?). Looks like code is semantically treating this as a list after the initial keyword? How about
reflect:/r/UDMI-REFLECT/d/+/state,/r/UDMI-REFLECT/d/+/events/#
So a colon after reflect -- which implies a list of things rather than a positional argument? Somehow that feels better/ok to me.
There was a problem hiding this comment.
We need to specify the two topics to not receive messages sent to the command topic. I didn't pay attention to what the first reflect is, admittedly I thought it was already there ... I think we can omit it completely and just have the two topics separated by a string
2 participants
The MQTT topics for sending
reflectmessages differs between the mqtt provider e.g.//mqtt/localhostand the gbos provider e.g. ``//gbos/project`.There is no reason for the difference apart. I believe the topic structure after the topic prefix should match within UDMI irrespective of the transport.
This PR aligns the two. The driver here is that when setting UDMIS to the
implicitprovider aka mosquitto, reflect clients connecting the currentgbosprovider do not work.Alternatively, we could just make the clients more flexible to support any authentication mechanism and topic structure, however as per above statement I don't think there should be a difference here.