You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2020/11/29 16:48:49 UTC
[GitHub] [ignite] nizhikov opened a new pull request #8509: [WIP] IGNITE-10075: Test to reproduce issue
nizhikov opened a new pull request #8509:
URL: https://github.com/apache/ignite/pull/8509
This PR contains a test to reproduce the described issue
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] ptupitsyn commented on a change in pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #8509:
URL: https://github.com/apache/ignite/pull/8509#discussion_r532856526
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/MarshallerContextImpl.java
##########
@@ -368,12 +369,27 @@ public void onMappingAccepted(final MarshallerMappingItem item) {
/** {@inheritDoc} */
@Override public Class getClass(int typeId, ClassLoader ldr) throws ClassNotFoundException, IgniteCheckedException {
- String clsName = getClassName(JAVA_ID, typeId);
+ String clsName;
+
+ ClassNotFoundException err = null;
+
+ for (byte platformId : new byte[] {JAVA_ID, DOTNET_ID}) {
Review comment:
`JAVA_ID` should come last - in most cases, platform type will be resolved, not a Java type.
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/binary/PlatformBinaryProcessor.java
##########
@@ -126,16 +128,28 @@ public PlatformBinaryProcessor(PlatformContext platformCtx) {
case OP_GET_TYPE: {
int typeId = reader.readInt();
- try {
- String typeName = platformContext().kernalContext().marshallerContext()
- .getClassName(MarshallerPlatformIds.DOTNET_ID, typeId);
+ ClassNotFoundException err = null;
- writer.writeString(typeName);
- }
- catch (ClassNotFoundException e) {
- throw new BinaryObjectException(e);
+ for (byte platformId : new byte[] {DOTNET_ID, JAVA_ID}) {
+ try {
+ String typeName = platformContext().kernalContext().marshallerContext()
+ .getClassName(platformId, typeId);
+
+ writer.writeString(typeName);
+
+ err = null;
+
+ break;
+ }
+ catch (ClassNotFoundException e) {
Review comment:
Let's not use exceptions for control flow - may be introduce an overload for `MarshallerContext.getClassName` that does not throw exceptions?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] ptupitsyn commented on pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on pull request #8509:
URL: https://github.com/apache/ignite/pull/8509#issuecomment-740072610
Looks good to me, thanks!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
nizhikov commented on pull request #8509:
URL: https://github.com/apache/ignite/pull/8509#issuecomment-738891681
Hello, @ptupitsyn
I've reworked this patch.
After some discussion with @kukushal I came to the decision that the initial patch (searching java type with .net name during deserialization) is bad.
Because we enhance global logic but not the logic that relates to the .net -> java service execution.
So I changed the algorithm to the following: during .net -> java service execution we register a new binary type both for java and .net platforms. We expect that there is java analog for each .Net type.
What do you think?
Can you, please, review the patch one more time.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
nizhikov commented on pull request #8509:
URL: https://github.com/apache/ignite/pull/8509#issuecomment-739994277
@ptupitsyn
> use a thread local flag in Marshaller
Implemented as suggested.
Please, take a look one more time.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
nizhikov commented on pull request #8509:
URL: https://github.com/apache/ignite/pull/8509#issuecomment-740030452
> I think it is a bit cleaner to pass the thread local flag as a parameter to the IBinaryProcessor.RegisterType
done.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] nizhikov merged pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
nizhikov merged pull request #8509:
URL: https://github.com/apache/ignite/pull/8509
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a change in pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8509:
URL: https://github.com/apache/ignite/pull/8509#discussion_r533248278
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/binary/PlatformBinaryProcessor.java
##########
@@ -126,16 +128,28 @@ public PlatformBinaryProcessor(PlatformContext platformCtx) {
case OP_GET_TYPE: {
int typeId = reader.readInt();
- try {
- String typeName = platformContext().kernalContext().marshallerContext()
- .getClassName(MarshallerPlatformIds.DOTNET_ID, typeId);
+ ClassNotFoundException err = null;
- writer.writeString(typeName);
- }
- catch (ClassNotFoundException e) {
- throw new BinaryObjectException(e);
+ for (byte platformId : new byte[] {DOTNET_ID, JAVA_ID}) {
+ try {
+ String typeName = platformContext().kernalContext().marshallerContext()
+ .getClassName(platformId, typeId);
+
+ writer.writeString(typeName);
+
+ err = null;
+
+ break;
+ }
+ catch (ClassNotFoundException e) {
Review comment:
I see your point, but if we introduce a method that not throws then we should return a tuple from it - T2(className, errorMsg).
Moreover, the second value of the tuple required in rare cases when we can't resolve the class name from type id.
AFAIU `getClassName` executed several times on every interaction between DotNet and Java so the creation of an extra object on each invocation will hurt the performance.
So I propose to keep changes as is. What do you think?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] ptupitsyn commented on pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on pull request #8509:
URL: https://github.com/apache/ignite/pull/8509#issuecomment-740015139
@nizhikov thanks, one more adjustment please:
I think it is a bit cleaner to pass the thread local flag as a parameter to the `IBinaryProcessor.RegisterType`, which is called from a single place in `Marshaller`. That way we reduce the global state usage.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] ptupitsyn commented on a change in pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #8509:
URL: https://github.com/apache/ignite/pull/8509#discussion_r533261346
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/binary/PlatformBinaryProcessor.java
##########
@@ -126,16 +128,28 @@ public PlatformBinaryProcessor(PlatformContext platformCtx) {
case OP_GET_TYPE: {
int typeId = reader.readInt();
- try {
- String typeName = platformContext().kernalContext().marshallerContext()
- .getClassName(MarshallerPlatformIds.DOTNET_ID, typeId);
+ ClassNotFoundException err = null;
- writer.writeString(typeName);
- }
- catch (ClassNotFoundException e) {
- throw new BinaryObjectException(e);
+ for (byte platformId : new byte[] {DOTNET_ID, JAVA_ID}) {
+ try {
+ String typeName = platformContext().kernalContext().marshallerContext()
+ .getClassName(platformId, typeId);
+
+ writer.writeString(typeName);
+
+ err = null;
+
+ break;
+ }
+ catch (ClassNotFoundException e) {
Review comment:
1. The cost of throwing an exception is **a lot** higher than a tuple allocation.
2. `getClassName` is called from .NET once per type, then cached
I'm not concerned about performance too much here because of (2). Exception-based control flow is just dirty.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a change in pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8509:
URL: https://github.com/apache/ignite/pull/8509#discussion_r533226059
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/MarshallerContextImpl.java
##########
@@ -368,12 +369,27 @@ public void onMappingAccepted(final MarshallerMappingItem item) {
/** {@inheritDoc} */
@Override public Class getClass(int typeId, ClassLoader ldr) throws ClassNotFoundException, IgniteCheckedException {
- String clsName = getClassName(JAVA_ID, typeId);
+ String clsName;
+
+ ClassNotFoundException err = null;
+
+ for (byte platformId : new byte[] {JAVA_ID, DOTNET_ID}) {
Review comment:
Fixed
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on a change in pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8509:
URL: https://github.com/apache/ignite/pull/8509#discussion_r533335826
##########
File path: modules/core/src/main/java/org/apache/ignite/internal/processors/platform/binary/PlatformBinaryProcessor.java
##########
@@ -126,16 +128,28 @@ public PlatformBinaryProcessor(PlatformContext platformCtx) {
case OP_GET_TYPE: {
int typeId = reader.readInt();
- try {
- String typeName = platformContext().kernalContext().marshallerContext()
- .getClassName(MarshallerPlatformIds.DOTNET_ID, typeId);
+ ClassNotFoundException err = null;
- writer.writeString(typeName);
- }
- catch (ClassNotFoundException e) {
- throw new BinaryObjectException(e);
+ for (byte platformId : new byte[] {DOTNET_ID, JAVA_ID}) {
+ try {
+ String typeName = platformContext().kernalContext().marshallerContext()
+ .getClassName(platformId, typeId);
+
+ writer.writeString(typeName);
+
+ err = null;
+
+ break;
+ }
+ catch (ClassNotFoundException e) {
Review comment:
Fixed according to your proposal.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [ignite] nizhikov commented on pull request #8509: IGNITE-10075 .NET: Avoid binary configurations of Ignite Java service params
Posted by GitBox <gi...@apache.org>.
nizhikov commented on pull request #8509:
URL: https://github.com/apache/ignite/pull/8509#issuecomment-736464264
> Are we sure that this is useful?
I and @kukushal have a request for a feature from one of the Ignite users that building an application that uses Java services from the DotNet platform.
----------------------------------------------------------------
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:
users@infra.apache.org