[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2163: IGNITE-19451 Prohibit some types from being Marshallable

["ibessonov (via GitHub)" <gi...@apache.org>]

ibessonov commented on code in PR #2163:
URL: https://github.com/apache/ignite-3/pull/2163#discussion_r1222916421


##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/IgniteTableImpl.java:
##########
@@ -369,6 +370,16 @@ public <RowT> CompletableFuture<?> upsertAll(
         return CompletableFuture.allOf(futures);
     }
 
+    private static List<ByteBuffer> serializeBinaryRows(Collection<BinaryRow> rows) {
+        var result = new ArrayList<ByteBuffer>(rows.size());
+
+        for (BinaryRow row : rows) {
+            result.add(row.byteBuffer());

Review Comment:
   In the future,  I guess we would like to remove this "byteBuffer" method. In that case it would be more convenient to have specific BinaryRowMessage class, or something similar, that would have schema version and the payload in different fields.
   Should we do it right now, or let's wait until the method is removed?



##########
modules/table/src/main/java/org/apache/ignite/internal/table/distributed/storage/InternalTableImpl.java:
##########
@@ -1429,6 +1454,7 @@ public void close() {
     }
 
     // TODO: IGNITE-17963 Use smarter logic for recipient node evaluation.
+

Review Comment:
   What's the meaning of this empty line and why the next javadoc is re-formatted? Was that done by IDEA?



##########
modules/network-annotation-processor/src/main/java/org/apache/ignite/internal/network/processor/messages/MarshallableTypesBlackList.java:
##########
@@ -97,11 +126,17 @@ public Boolean visitDeclared(DeclaredType t, Void unused) {
                 return t.getTypeArguments().stream().anyMatch(this::visit);
             }
 
-            return !isSameType(NATIVE_TYPES, t) && !typeUtils.isSubType(t, NetworkMessage.class);
+            return !isSameType(NATIVE_TYPES, t)
+                    && !typeUtils.isSubType(t, NetworkMessage.class)

Review Comment:
   Would it make sense to implement NetworkMassage and default types as a blacklist file themselves?



-- 
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: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org