stankiewicz commented on code in PR #40108:
URL: https://github.com/apache/beam/pull/40108#discussion_r4003888699


##########
sdks/java/io/solace/src/main/java/org/apache/beam/sdk/io/solace/data/Solace.java:
##########
@@ -599,5 +623,47 @@ private static byte[] readAttachment(BytesXMLMessage msg) {
       buffer.get(attachment);
       return attachment;
     }
+
+    private static Map<String, String> getUserProperties(@Nullable SDTMap 
properties) {
+      if (properties == null || properties.isEmpty()) {
+        return Collections.emptyMap();
+      }
+
+      Map<String, String> userProperties = new HashMap<>();
+      for (String key : properties.keySet()) {
+        String value = stringifyUserProperty(properties, key);
+        if (value == null) {
+          LOG.warn("User property '{}' has a null value, skipping.", key);
+          continue;
+        }
+        userProperties.put(key, value);
+      }
+      return Collections.unmodifiableMap(userProperties);
+    }
+
+    private static @Nullable String stringifyUserProperty(SDTMap properties, 
String key) {
+      try {
+        Object value = properties.get(key);
+        if (value == null) {
+          return null;
+        }
+        return String.valueOf(value);

Review 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
   



##########
sdks/java/io/solace/src/main/java/org/apache/beam/sdk/io/solace/data/Solace.java:
##########
@@ -599,5 +623,47 @@ private static byte[] readAttachment(BytesXMLMessage msg) {
       buffer.get(attachment);
       return attachment;
     }
+
+    private static Map<String, String> getUserProperties(@Nullable SDTMap 
properties) {
+      if (properties == null || properties.isEmpty()) {
+        return Collections.emptyMap();
+      }
+
+      Map<String, String> userProperties = new HashMap<>();
+      for (String key : properties.keySet()) {
+        String value = stringifyUserProperty(properties, key);
+        if (value == null) {
+          LOG.warn("User property '{}' has a null value, skipping.", key);
+          continue;
+        }
+        userProperties.put(key, value);
+      }
+      return Collections.unmodifiableMap(userProperties);
+    }
+
+    private static @Nullable String stringifyUserProperty(SDTMap properties, 
String key) {
+      try {
+        Object value = properties.get(key);
+        if (value == null) {
+          return null;
+        }
+        return String.valueOf(value);
+      } catch (SDTException e) {
+        LOG.error("Could not read user property '{}'.", key, e);

Review Comment:
   @ngibanel this will cause metadata loss as message will be acked.  Maybe 
rethrowing will be better?



##########
sdks/java/io/solace/src/test/java/org/apache/beam/sdk/io/solace/data/SolaceRecordMapperTest.java:
##########
@@ -144,6 +148,34 @@ public void testMapMessageMetadata() {
     assertEquals(789L, record.getTimeToLive());
   }
 
+  @Test
+  public void testMapMessageUserProperties() throws Exception {
+    BytesXMLMessage message = 
JCSMPFactory.onlyInstance().createBytesXMLMessage();
+    message.setApplicationMessageId("id");
+    SDTMap properties = JCSMPFactory.onlyInstance().createMap();
+    properties.putString("contentType", "application/json");

Review Comment:
   cover all SDTMap types



##########
sdks/java/io/solace/src/main/java/org/apache/beam/sdk/io/solace/data/Solace.java:
##########
@@ -599,5 +623,47 @@ private static byte[] readAttachment(BytesXMLMessage msg) {
       buffer.get(attachment);
       return attachment;
     }
+
+    private static Map<String, String> getUserProperties(@Nullable SDTMap 
properties) {
+      if (properties == null || properties.isEmpty()) {
+        return Collections.emptyMap();
+      }
+
+      Map<String, String> userProperties = new HashMap<>();
+      for (String key : properties.keySet()) {
+        String value = stringifyUserProperty(properties, key);
+        if (value == null) {
+          LOG.warn("User property '{}' has a null value, skipping.", key);

Review Comment:
   this may be excessive, would skip this log.   



-- 
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]

Reply via email to