Conversation
51b798d to
ac66e7b
Compare
ac66e7b to
c75a283
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #40108 +/- ##
============================================
- Coverage 58.72% 58.41% -0.32%
+ Complexity 15295 13571 -1724
============================================
Files 2791 2577 -214
Lines 278417 269098 -9319
Branches 12320 11058 -1262
============================================
- Hits 163512 157183 -6329
+ Misses 108500 105961 -2539
+ Partials 6405 5954 -451
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
R: @stankiewicz |
|
Stopping reviewer notifications for this pull request: review requested by someone other than the bot, ceding control. If you'd like to restart, comment |
stankiewicz
left a comment
stankiewicz
left a comment
There was a problem hiding this comment.
Thanks Nicolas, requested few changes, ptal!
| } | ||
| return String.valueOf(value); | ||
| } catch (SDTException e) { | ||
| LOG.error("Could not read user property '{}'.", key, e); |
There was a problem hiding this comment.
@ngibanel this will cause metadata loss as message will be acked. Maybe rethrowing will be better?
There was a problem hiding this comment.
ok, that means the consumer must enable the dead message queue in Solace (which is a best practice) to avoid losing messages if metadata cannot be deserialized. Otherwise the broker will redeliver the message until the max retry count is reached and then message will be discarded and lost.
| if (value == null) { | ||
| return null; | ||
| } | ||
| return String.valueOf(value); |
There was a problem hiding this comment.
this will invoke toString but some types (destination, stream, byte array) that are part of SDTMap don't have it and this will run poorly for those, some specialized approach should be used for those like
Destination type maybe getName() should be invoked and for Stream maybe byte array and for byte array you should somehow preserve those bytes so its' not becoming garbage
There was a problem hiding this comment.
good catch I didn't check all the types. Maybe translating all types into string is not the good choice at the end because with this design we won't be able to reverse to Solace types.
@stankiewicz what do you think if instead having a Map<String, String>, having a Map<String, UserPropertyValue> where UserPropertyValue will be a beam schema compatible model that supports all the type kinds :
@AutoValue
@DefaultSchema(AutoValueSchema.class)
public abstract static class UserPropertyValue {
public enum Kind {
BOOLEAN,
BYTE,
SHORT,
INTEGER,
LONG,
FLOAT,
DOUBLE,
CHARACTER,
STRING,
BYTES,
TOPIC,
QUEUE,
MAP,
STREAM
}
public abstract Kind getKind();
public abstract @Nullable Map<String, UserPropertyValue> getMapValue();
public abstract @Nullable List<UserPropertyValue> getStreamValue();
...
}| for (String key : properties.keySet()) { | ||
| String value = stringifyUserProperty(properties, key); | ||
| if (value == null) { | ||
| LOG.warn("User property '{}' has a null value, skipping.", key); |
There was a problem hiding this comment.
this may be excessive, would skip this log.
| BytesXMLMessage message = JCSMPFactory.onlyInstance().createBytesXMLMessage(); | ||
| message.setApplicationMessageId("id"); | ||
| SDTMap properties = JCSMPFactory.onlyInstance().createMap(); | ||
| properties.putString("contentType", "application/json"); |
There was a problem hiding this comment.
cover all SDTMap types
2 participants
Summary
SolaceIO now supports user properties (message metadata) mapping in both the read and the
write direction.
Fixes #40099
Motivation
The SolaceIO serialization layer (
Solace.SolaceRecordMapper) currently drops the JCSMP userproperty map (
XMLMessage.getProperties(), anSDTMap) in both directions:toRecorddoes not mapgetProperties()intoSolace.Record, so allheader-level metadata carried by the message is lost when reading from Solace.
toMessagenever callssetProperties(SDTMap), so it is impossible to publishuser properties from a Beam pipeline.
User properties are the recommended way to carry small header-level metadata fields alongside
the payload. They are also the mechanism that interoperates across protocols: Solace translates
the SDTMap user property map to/from MQTT 5 user properties and AMQP application properties.
Supporting them enables cross-protocol (MQTT 5 / AMQP / SMF) metadata interoperability from
Beam pipelines.
This is a follow-up to #39875, reusing the same mapper/protocol layer introduced for
payload type support.
Changes
Solace.Recordgains auserPropertiesfield of typeMap<String, String>(schema field 14).It defaults to an empty map and is never null, so application code does not need null handling.
toRecord): each entry of the JCSMPSDTMapis stringified(
String.valueOf) into the record. Entries with a null value are skipped with a warning;entries that cannot be read are logged and skipped. A message without user properties maps
to an empty map.
toMessage): when the record carries a non-emptyuserPropertiesmap,it is published as a JCSMP
SDTMapviasetProperties(...). When the map is empty,setPropertiesis not called, so published messages are identical to the previous behavior.properties are strings. Users needing the original non-string types can cast the values back
in their pipeline code. Typed SDT values are out of scope.
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>instead.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.