You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "sashapolo (via GitHub)" <gi...@apache.org> on 2023/06/07 14:01:52 UTC
[GitHub] [ignite-3] sashapolo opened a new pull request, #2163: IGNITE-19451 Prohibit some types from being Marshallable
sashapolo opened a new pull request, #2163:
URL: https://github.com/apache/ignite-3/pull/2163
https://issues.apache.org/jira/browse/IGNITE-19451
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2163: IGNITE-19451 Prohibit some types from being Marshallable
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2163:
URL: https://github.com/apache/ignite-3/pull/2163#discussion_r1222962531
##########
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:
Because what's the point of making a copy of an array if all we want is to slap a single integer to it. That's extra copying that most likely affects the performance in negative way.
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #2163: IGNITE-19451 Prohibit some types from being Marshallable
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2163:
URL: https://github.com/apache/ignite-3/pull/2163#discussion_r1222956009
##########
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:
I don't want to, because we will have to duplicate them in the test list as well, plus plain class names are less convenient
##########
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:
I don't want to, because we will have to duplicate them in the test list as well, text plain class names are less convenient
##########
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:
I don't want to, because we will have to duplicate them in the test list as well, text class names are less convenient
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #2163: IGNITE-19451 Prohibit some types from being Marshallable
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2163:
URL: https://github.com/apache/ignite-3/pull/2163#discussion_r1222953774
##########
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:
yes, I've applied auto-formatting
--
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
[GitHub] [ignite-3] ibessonov merged pull request #2163: IGNITE-19451 Prohibit some types from being Marshallable
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov merged PR #2163:
URL: https://github.com/apache/ignite-3/pull/2163
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #2163: IGNITE-19451 Prohibit some types from being Marshallable
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2163:
URL: https://github.com/apache/ignite-3/pull/2163#discussion_r1222965514
##########
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:
> Should we do it right now, or let's wait until the method is removed?
Let's do it later, I don't want to make stuff more complex prematurely
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #2163: IGNITE-19451 Prohibit some types from being Marshallable
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2163:
URL: https://github.com/apache/ignite-3/pull/2163#discussion_r1222956009
##########
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:
I don't want to, because we will have to duplicate them in the test list as well, and text class names are less convenient
--
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
[GitHub] [ignite-3] sashapolo commented on a diff in pull request #2163: IGNITE-19451 Prohibit some types from being Marshallable
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #2163:
URL: https://github.com/apache/ignite-3/pull/2163#discussion_r1222954394
##########
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.
Why?
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #2163: IGNITE-19451 Prohibit some types from being Marshallable
Posted by "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