mynameborat commented on a change in pull request #1468:
URL: https://github.com/apache/samza/pull/1468#discussion_r586148791



##########
File path: 
samza-core/src/main/java/org/apache/samza/execution/JobNodeConfigurationGenerator.java
##########
@@ -351,10 +351,10 @@ private void configureSerdes(Map<String, String> configs, 
Map<String, StreamEdge
   private void addSerdes(KV<Serde, Serde> serdes, String streamId, Map<String, 
Serde> keySerdeMap,
       Map<String, Serde> msgSerdeMap) {
     if (serdes != null) {
-      if (serdes.getKey() != null && !(serdes.getKey() instanceof NoOpSerde)) {
+      if (serdes.getKey() != null) {
         keySerdeMap.put(streamId, serdes.getKey());
       }
-      if (serdes.getValue() != null && !(serdes.getValue() instanceof 
NoOpSerde)) {
+      if (serdes.getValue() != null) {

Review comment:
       This will impact the maps being returned to the caller which can now 
have multiple instances of no-op serde even though in theory all of them are 
logically the same.
   
   While this may not impact functionality, it does create redundant copies of 
serialized serdes & configurations.




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

For queries about this service, please contact Infrastructure at:
[email protected]


Reply via email to