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 2021/01/07 10:56:36 UTC
[GitHub] [ignite] nizhikov opened a new pull request #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute and Cache API
nizhikov opened a new pull request #8635:
URL: https://github.com/apache/ignite/pull/8635
This PR enables an implicit binary type registration on the invocation of Compute and Cache API.
### 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] nizhikov commented on a change in pull request #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r560056162
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Services/Services.cs
##########
@@ -434,7 +434,7 @@ public dynamic GetDynamicServiceProxy(string name, bool sticky)
private object InvokeProxyMethod(IPlatformTargetInternal proxy, string methodName,
MethodBase method, object[] args, PlatformType platformType)
{
- Marshaller.RegisterSameJavaType.Value = true;
+ Marshaller.RegisterSameJavaTypeTl.Value = true;
Review comment:
Fixed
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Services/Services.cs
##########
@@ -445,7 +445,7 @@ public dynamic GetDynamicServiceProxy(string name, bool sticky)
}
finally
{
- Marshaller.RegisterSameJavaType.Value = false;
+ Marshaller.RegisterSameJavaTypeTl.Value = false;
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r559752247
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/Marshaller.cs
##########
@@ -152,6 +159,14 @@ public ITimestampConverter TimestampConverter
get { return _cfg.TimestampConverter; }
}
+ /// <summary>
+ /// Returns register same java type flag value.
+ /// </summary>
+ public bool IsRegisterSameJavaType()
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r559961249
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/BinaryProcessorClient.cs
##########
@@ -99,11 +105,38 @@ public BinaryType RegisterEnum(string typeName, IEnumerable<KeyValuePair<string,
}
/** <inheritdoc /> */
- public string GetTypeName(int id)
+ public string GetTypeName(int id, bool registerSameJavaType)
+ {
+ try
+ {
+ return GetTypeName(id, BinaryProcessor.DotNetPlatformId);
+ }
+ catch (Exception)
Review comment:
Fixed.
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/PlatformJniTarget.cs
##########
@@ -309,7 +309,8 @@ private void FinishMarshal(BinaryWriter writer)
var fut = convertFunc == null && futType != FutureType.Object
? new Future<T>()
- : new Future<T>(new FutureConverter<T>(_marsh, keepBinary, convertFunc));
+ : new Future<T>(new FutureConverter<T>(_marsh, keepBinary, _marsh.IsRegisterSameJavaType(),
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 merged pull request #8635: IGNITE-13958 .NET: Add implicit Java type registration in ExecuteJavaTask
Posted by GitBox <gi...@apache.org>.
nizhikov merged pull request #8635:
URL: https://github.com/apache/ignite/pull/8635
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r560046758
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/BinaryProcessor.cs
##########
@@ -165,41 +167,39 @@ public BinaryType RegisterEnum(string typeName, IEnumerable<KeyValuePair<string,
/// Gets the type name by id.
/// </summary>
/// <param name="id">The identifier.</param>
+ /// <param name="registerSameJavaType">True if should register type both for dotnet and java platforms.</param>
/// <returns>Type or null.</returns>
- public string GetTypeName(int id)
+ public string GetTypeName(int id, bool registerSameJavaType)
{
- try
+ return GetTypeName(id, DotNetPlatformId, ex =>
{
- return GetTypeName(id, DotNetPlatformId);
- }
- catch (BinaryObjectException)
- {
- if (!Marshaller.RegisterSameJavaType.Value)
- throw;
- }
+ if (!registerSameJavaType)
+ throw ex;
- // Try to get java type name and register corresponding DotNet type.
- var javaTypeName = GetTypeName(id, JavaPlatformId);
- var netTypeName = Marshaller.GetTypeName(javaTypeName);
+ // Try to get java type name and register corresponding DotNet type.
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute and Cache API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r559413768
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/Marshaller.cs
##########
@@ -109,6 +108,10 @@ public Marshaller(BinaryConfiguration cfg, ILogger log = null)
if (typeNames != null)
foreach (string typeName in typeNames)
AddUserType(new BinaryTypeConfiguration(typeName), typeResolver);
+
+ _registerSameJavaType = _cfg.NameMapper == null ||
Review comment:
It seems I get you wrong in the dev-list discussion.
Do you suggest to avoid implicit type registration for cache operations?
I think it's questionable from the user's point of view.
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#issuecomment-762743549
@ptupitsyn All comments are fixed. Please, take a look.
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r559753191
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/BinaryProcessor.cs
##########
@@ -165,16 +165,17 @@ public BinaryType RegisterEnum(string typeName, IEnumerable<KeyValuePair<string,
/// Gets the type name by id.
/// </summary>
/// <param name="id">The identifier.</param>
+ /// <param name="registerSameJavaType">True if should register type both for dotnet and java platforms.</param>
/// <returns>Type or null.</returns>
- public string GetTypeName(int id)
+ public string GetTypeName(int id, bool registerSameJavaType)
{
try
{
return GetTypeName(id, DotNetPlatformId);
}
catch (BinaryObjectException)
{
- if (!Marshaller.RegisterSameJavaType.Value)
+ if (!registerSameJavaType)
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r560026067
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/PlatformJniTarget.cs
##########
@@ -177,6 +177,9 @@ public T OutStream<T>(int type, Func<IBinaryStream, T> readAction)
}
catch (JavaException jex)
{
+ if (errorAction != null)
+ return errorAction.Invoke(jex);
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] ptupitsyn commented on pull request #8635: IGNITE-13958 .NET: Add implicit Java type registration in ExecuteJavaTask
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#issuecomment-762781979
@nizhikov looks good to me, thank you! I've pushed some minor tweaks - please check if you are ok with them.
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute and Cache API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r555281005
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Common/FutureConverter.cs
##########
@@ -35,28 +35,43 @@ internal class FutureConverter<T> : IFutureConverter<T>
/** Converting function. */
private readonly Func<BinaryReader, T> _func;
+ /** True if should register type both for dotnet and java platforms. */
+ private readonly bool _registerSameJavaType;
+
/// <summary>
/// Constructor.
/// </summary>
/// <param name="marsh">Marshaller.</param>
/// <param name="keepBinary">Keep binary flag.</param>
/// <param name="func">Converting function.</param>
+ /// <param name="registerSameJavaType">True if should register type both for dotnet and java platforms.</param>
public FutureConverter(Marshaller marsh, bool keepBinary,
- Func<BinaryReader, T> func = null)
+ Func<BinaryReader, T> func = null, bool registerSameJavaType = false)
{
_marsh = marsh;
_keepBinary = keepBinary;
_func = func ?? (reader => reader == null ? default(T) : reader.ReadObject<T>());
+ _registerSameJavaType = registerSameJavaType;
}
/// <summary>
/// Read and convert a value.
/// </summary>
public T Convert(IBinaryStream stream)
{
- var reader = stream == null ? null : _marsh.StartUnmarshal(stream, _keepBinary);
+ var locRegisterSameJavaType = Marshaller.RegisterSameJavaType.Value;
+ Marshaller.RegisterSameJavaType.Value = _registerSameJavaType;
+
+ try
+ {
+ var reader = stream == null ? null : _marsh.StartUnmarshal(stream, _keepBinary);
Review comment:
These changes required for reading binary object of unregistered type and dynamically resolve those type.
Please, take a look at `ComputeApiTypeAutoRegisterTest#TestEchoTasksAutoRegisterType`
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute and Cache API
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r559423730
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/Marshaller.cs
##########
@@ -109,6 +108,10 @@ public Marshaller(BinaryConfiguration cfg, ILogger log = null)
if (typeNames != null)
foreach (string typeName in typeNames)
AddUserType(new BinaryTypeConfiguration(typeName), typeResolver);
+
+ _registerSameJavaType = _cfg.NameMapper == null ||
Review comment:
Yes, I suggest to register Java classes only when Java code is being invoked with `ExecuteJavaTask` or Java service call. I do not think it is necessary for cache operations. Let's get back on the dev list if you'd like to discuss further.
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute and Cache API
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r559389578
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/Marshaller.cs
##########
@@ -109,6 +108,10 @@ public Marshaller(BinaryConfiguration cfg, ILogger log = null)
if (typeNames != null)
foreach (string typeName in typeNames)
AddUserType(new BinaryTypeConfiguration(typeName), typeResolver);
+
+ _registerSameJavaType = _cfg.NameMapper == null ||
Review comment:
As we agreed on the [dev list](http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSSION-Net-BinaryTypes-transparency-tp50998p51029.html), `_registerSameJavaType` should take effect only within Java service call or `ExecuteJavaTask`.
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/BinaryProcessorClient.cs
##########
@@ -99,11 +105,38 @@ public BinaryType RegisterEnum(string typeName, IEnumerable<KeyValuePair<string,
}
/** <inheritdoc /> */
- public string GetTypeName(int id)
+ public string GetTypeName(int id, bool registerSameJavaType)
+ {
+ try
+ {
+ return GetTypeName(id, BinaryProcessor.DotNetPlatformId);
+ }
+ catch (Exception)
Review comment:
Same as above - please don't use exceptions for control flow. For thin client we can use `errorFunc` parameter of the `DoOutInOp` function to handle errors without throwing exceptions.
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/BinaryProcessor.cs
##########
@@ -165,16 +165,17 @@ public BinaryType RegisterEnum(string typeName, IEnumerable<KeyValuePair<string,
/// Gets the type name by id.
/// </summary>
/// <param name="id">The identifier.</param>
+ /// <param name="registerSameJavaType">True if should register type both for dotnet and java platforms.</param>
/// <returns>Type or null.</returns>
- public string GetTypeName(int id)
+ public string GetTypeName(int id, bool registerSameJavaType)
{
try
{
return GetTypeName(id, DotNetPlatformId);
}
catch (BinaryObjectException)
{
- if (!Marshaller.RegisterSameJavaType.Value)
+ if (!registerSameJavaType)
Review comment:
Let's avoid using exceptions for control flow. At least we can push the flag to `PlatformBinaryProcessor.OP_GET_TYPE` handler, which already has an exception handler.
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Add implicit Java type registration in ExecuteJavaTask
Posted by GitBox <gi...@apache.org>.
nizhikov commented on pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#issuecomment-762783842
@ptupitsyn Thanks for the help. Appreciate it.
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r560019896
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/Marshaller.cs
##########
@@ -582,7 +597,7 @@ private BinaryFullTypeDescriptor RegisterType(Type type, BinaryFullTypeDescripto
var typeName = GetTypeName(type);
var typeId = GetTypeId(typeName, _cfg.IdMapper);
- var registered = _ignite != null && _ignite.BinaryProcessor.RegisterType(typeId, typeName, RegisterSameJavaType.Value);
+ var registered = _ignite != null && _ignite.BinaryProcessor.RegisterType(typeId, typeName, _registerSameJavaType);
Review comment:
Good catch. Fixed. 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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute and Cache API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#issuecomment-762041357
Hello @ptupitsyn
PR updated according to discussion on dev-list.
Can you, please, take a look?
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute and Cache API
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r553476265
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Common/FutureConverter.cs
##########
@@ -35,28 +35,43 @@ internal class FutureConverter<T> : IFutureConverter<T>
/** Converting function. */
private readonly Func<BinaryReader, T> _func;
+ /** True if should register type both for dotnet and java platforms. */
+ private readonly bool _registerSameJavaType;
+
/// <summary>
/// Constructor.
/// </summary>
/// <param name="marsh">Marshaller.</param>
/// <param name="keepBinary">Keep binary flag.</param>
/// <param name="func">Converting function.</param>
+ /// <param name="registerSameJavaType">True if should register type both for dotnet and java platforms.</param>
public FutureConverter(Marshaller marsh, bool keepBinary,
- Func<BinaryReader, T> func = null)
+ Func<BinaryReader, T> func = null, bool registerSameJavaType = false)
{
_marsh = marsh;
_keepBinary = keepBinary;
_func = func ?? (reader => reader == null ? default(T) : reader.ReadObject<T>());
+ _registerSameJavaType = registerSameJavaType;
}
/// <summary>
/// Read and convert a value.
/// </summary>
public T Convert(IBinaryStream stream)
{
- var reader = stream == null ? null : _marsh.StartUnmarshal(stream, _keepBinary);
+ var locRegisterSameJavaType = Marshaller.RegisterSameJavaType.Value;
+ Marshaller.RegisterSameJavaType.Value = _registerSameJavaType;
+
+ try
+ {
+ var reader = stream == null ? null : _marsh.StartUnmarshal(stream, _keepBinary);
Review comment:
I think all changes in this file are unnecessary, since we don't write anything, only read.
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r559529745
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/Marshaller.cs
##########
@@ -109,6 +108,10 @@ public Marshaller(BinaryConfiguration cfg, ILogger log = null)
if (typeNames != null)
foreach (string typeName in typeNames)
AddUserType(new BinaryTypeConfiguration(typeName), typeResolver);
+
+ _registerSameJavaType = _cfg.NameMapper == null ||
Review comment:
OK. I removed changes for Cache API.
We will discuss it later in the follow-ups of this PR.
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r560048124
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/BinaryProcessor.cs
##########
@@ -165,41 +167,39 @@ public BinaryType RegisterEnum(string typeName, IEnumerable<KeyValuePair<string,
/// Gets the type name by id.
/// </summary>
/// <param name="id">The identifier.</param>
+ /// <param name="registerSameJavaType">True if should register type both for dotnet and java platforms.</param>
/// <returns>Type or null.</returns>
- public string GetTypeName(int id)
+ public string GetTypeName(int id, bool registerSameJavaType)
{
- try
+ return GetTypeName(id, DotNetPlatformId, ex =>
{
- return GetTypeName(id, DotNetPlatformId);
- }
- catch (BinaryObjectException)
- {
- if (!Marshaller.RegisterSameJavaType.Value)
- throw;
- }
+ if (!registerSameJavaType)
+ throw ex;
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 pull request #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
nizhikov commented on pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#issuecomment-762655556
@ptupitsyn I fixed all your comments. 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] ptupitsyn commented on a change in pull request #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r559591660
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/PlatformJniTarget.cs
##########
@@ -309,7 +309,8 @@ private void FinishMarshal(BinaryWriter writer)
var fut = convertFunc == null && futType != FutureType.Object
? new Future<T>()
- : new Future<T>(new FutureConverter<T>(_marsh, keepBinary, convertFunc));
+ : new Future<T>(new FutureConverter<T>(_marsh, keepBinary, _marsh.IsRegisterSameJavaType(),
Review comment:
We pass `_marsh` to the constructor, no need to pass a property of `_marsh` separately (here and below)
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/Marshaller.cs
##########
@@ -152,6 +159,14 @@ public ITimestampConverter TimestampConverter
get { return _cfg.TimestampConverter; }
}
+ /// <summary>
+ /// Returns register same java type flag value.
+ /// </summary>
+ public bool IsRegisterSameJavaType()
Review comment:
Nitpick: replace this method with a property
----------------------------------------------------------------
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 #8635: IGNITE-13958 .NET: Implicit binary type registration in Compute API
Posted by GitBox <gi...@apache.org>.
ptupitsyn commented on a change in pull request #8635:
URL: https://github.com/apache/ignite/pull/8635#discussion_r560000364
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/Marshaller.cs
##########
@@ -582,7 +597,7 @@ private BinaryFullTypeDescriptor RegisterType(Type type, BinaryFullTypeDescripto
var typeName = GetTypeName(type);
var typeId = GetTypeId(typeName, _cfg.IdMapper);
- var registered = _ignite != null && _ignite.BinaryProcessor.RegisterType(typeId, typeName, RegisterSameJavaType.Value);
+ var registered = _ignite != null && _ignite.BinaryProcessor.RegisterType(typeId, typeName, _registerSameJavaType);
Review comment:
`_registerSameJavaType` -> `RegisterSameJavaType`
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/PlatformJniTarget.cs
##########
@@ -177,6 +177,9 @@ public T OutStream<T>(int type, Func<IBinaryStream, T> readAction)
}
catch (JavaException jex)
{
+ if (errorAction != null)
+ return errorAction.Invoke(jex);
Review comment:
I think this should be `return errorAction.Invoke(ConvertException(jex));`. Otherwise, when `errorAction` rethrows the exception, it is not converted properly.
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Services/Services.cs
##########
@@ -445,7 +445,7 @@ public dynamic GetDynamicServiceProxy(string name, bool sticky)
}
finally
{
- Marshaller.RegisterSameJavaType.Value = false;
+ Marshaller.RegisterSameJavaTypeTl.Value = false;
Review comment:
Let's preserve the previous thread-local value instead of assuming it to be false. Nested .NET -> Java -> .NET -> Java -> ... calls are possible.
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/BinaryProcessor.cs
##########
@@ -165,41 +167,39 @@ public BinaryType RegisterEnum(string typeName, IEnumerable<KeyValuePair<string,
/// Gets the type name by id.
/// </summary>
/// <param name="id">The identifier.</param>
+ /// <param name="registerSameJavaType">True if should register type both for dotnet and java platforms.</param>
/// <returns>Type or null.</returns>
- public string GetTypeName(int id)
+ public string GetTypeName(int id, bool registerSameJavaType)
{
- try
+ return GetTypeName(id, DotNetPlatformId, ex =>
{
- return GetTypeName(id, DotNetPlatformId);
- }
- catch (BinaryObjectException)
- {
- if (!Marshaller.RegisterSameJavaType.Value)
- throw;
- }
+ if (!registerSameJavaType)
+ throw ex;
Review comment:
Stack trace gets lost when we rethrow an exception like this. It should be wrapped in another exception, e.g. `new BinaryObjectException("Unknown type ID: " + id, ex)`
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Binary/BinaryProcessor.cs
##########
@@ -165,41 +167,39 @@ public BinaryType RegisterEnum(string typeName, IEnumerable<KeyValuePair<string,
/// Gets the type name by id.
/// </summary>
/// <param name="id">The identifier.</param>
+ /// <param name="registerSameJavaType">True if should register type both for dotnet and java platforms.</param>
/// <returns>Type or null.</returns>
- public string GetTypeName(int id)
+ public string GetTypeName(int id, bool registerSameJavaType)
{
- try
+ return GetTypeName(id, DotNetPlatformId, ex =>
{
- return GetTypeName(id, DotNetPlatformId);
- }
- catch (BinaryObjectException)
- {
- if (!Marshaller.RegisterSameJavaType.Value)
- throw;
- }
+ if (!registerSameJavaType)
+ throw ex;
- // Try to get java type name and register corresponding DotNet type.
- var javaTypeName = GetTypeName(id, JavaPlatformId);
- var netTypeName = Marshaller.GetTypeName(javaTypeName);
+ // Try to get java type name and register corresponding DotNet type.
Review comment:
This retry logic is duplicated in `BinaryProcessor` and `BinaryProcessorClient`. Let's move this to the `Marshaller` class.
##########
File path: modules/platforms/dotnet/Apache.Ignite.Core/Impl/Services/Services.cs
##########
@@ -434,7 +434,7 @@ public dynamic GetDynamicServiceProxy(string name, bool sticky)
private object InvokeProxyMethod(IPlatformTargetInternal proxy, string methodName,
MethodBase method, object[] args, PlatformType platformType)
{
- Marshaller.RegisterSameJavaType.Value = true;
+ Marshaller.RegisterSameJavaTypeTl.Value = true;
Review comment:
`if (platformType == PlatformType.Java)`
----------------------------------------------------------------
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