ignite-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ivan Bessonov (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (IGNITE-11397) Binary mode for Ignite Set
Date Mon, 01 Apr 2019 11:18:02 GMT

    [ https://issues.apache.org/jira/browse/IGNITE-11397?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16806530#comment-16806530
] 

Ivan Bessonov edited comment on IGNITE-11397 at 4/1/19 11:18 AM:
-----------------------------------------------------------------

Hello [~uday], thank you for the contribution! I'd like to discuss few things here.

First of all, these tests failures look very suspicious, at least in "Data Structures" suite.
Please take a look at them.

 

Now more specific. In modified abstract test you have this line:
{code:java}
private static final boolean BINARY_SET_MODE = IgniteSystemProperties.getBoolean(BINARY_SET,
true);{code}
You set "true" as a default value and you also set "true" in suite. I can't find a place where
you would use "false" as a value for this property, please correct that.

 
{code:java}
public <V1> IgniteSet<V1> withKeepBinary();{code}
I understand where this type of method signature is coming from, but I'd like to be sure that
it's correct. There are two implementations of this method and they have following lines in
them:
{code:java}
return new GridCacheSetProxy<>(ctx, (GridCacheSetImpl<V1>)this);{code}
{code:java}
return new GridCacheSetProxy<>(cctx, (GridCacheSetImpl<V1>)delegate);{code}
Both "this" and "delegate" are instances of "IgniteSet<T>" so why do we need a new type
"V1" that is not even bound with anything? Looks problematic to say the least. On the other
hand, actual type when operating these sets is most likely a "BinaryObject". If it is then
we could reflect this fact in method signature so it won't be overly complicated.

 

Finally, you mishandled "CacheOperationContext". Please take a look at "GatewayProtectedCacheProxy"
class and usages of "operationContextPerCall" method. What you actually need is to have "opCtx"
field in "GridCacheSetProxy" and pass it into the gate. Please don't forget to restore previous
values on leaving the gate, like, for example, in "GatewayProtectedCacheProxy.CacheOperationGate"
class usages.

Here is the simple reproducer for the problem. It doesn't cover all the aspects but is enough
for the start:
{code:java}
@Test
public void testGet() throws Exception {
    IgniteEx ignite = grid(0);

    IgniteQueue<BinaryObject> queue = ignite.queue("q0", 10, config(false)).withKeepBinary();

    queue.add(sameHashBinObj(ignite, 0));
    queue.add(sameHashBinObj(ignite, 1));

    BinaryObject o0 = queue.poll(); // Ok.

    BinaryObject o1 = GridTestUtils.runAsync((Callable<BinaryObject>)queue::poll).get();
// Fails.
}

private static BinaryObject sameHashBinObj(Ignite ignite, int i) {
    return ignite.binary().toBinary(new SameHashItem(Integer.toString(i)));
}
{code}
You probably noticed that this test operates with "IgniteQueue". It has all the same issues
so they can be reproduced in master. I would appreciate if you created another issue to fix
these bugs in "IgniteQueue" after we finish "IgniteSet" improvements.

 

Please feel free asking me more questions if you have some. Thank you!

 

EDIT:

I feel like I didn't explain that last problem detailed enough so I'll do it just in case.

"operationContextPerCall" is a thread-local entity used for cache operations. It is set before
any specific cache operation (get / put / ...) and cleared after. This way cache has the ability
to read all specific settings and work as desired. More specifically - it allows having both
"keepBinary" and "not keepBinary" data structures on the same cache in the same thread.

In current implementation of queue and suggested implementation of set you don't use "operationContextPerCall"
in the expected way (try/finally around specific operations) but rather just set it in the
moment of creation. All queues and sets become "keepBinary" structures in current thread but
remain "not keepBinary" in other (unless those threads are not broken like the current one).
This is the exact scenario that I provided in my test (loosely based on your test from this
comment).


was (Author: ibessonov):
Hello [~uday], thank you for the contribution! I'd like to discuss few things here.

First of all, these tests failures look very suspicious, at least in "Data Structures" suite.
Please take a look at them.

 

Now more specific. In modified abstract test you have this line:
{code:java}
private static final boolean BINARY_SET_MODE = IgniteSystemProperties.getBoolean(BINARY_SET,
true);{code}
You set "true" as a default value and you also set "true" in suite. I can't find a place where
you would use "false" as a value for this property, please correct that.

 
{code:java}
public <V1> IgniteSet<V1> withKeepBinary();{code}
I understand where this type of method signature is coming from, but I'd like to be sure that
it's correct. There are two implementations of this method and they have following lines in
them:
{code:java}
return new GridCacheSetProxy<>(ctx, (GridCacheSetImpl<V1>)this);{code}
{code:java}
return new GridCacheSetProxy<>(cctx, (GridCacheSetImpl<V1>)delegate);{code}
Both "this" and "delegate" are instances of "IgniteSet<T>" so why do we need a new type
"V1" that is not even bound with anything? Looks problematic to say the least. On the other
hand, actual type when operating these sets is most likely a "BinaryObject". If it is then
we could reflect this fact in method signature so it won't be overly complicated.

 

Finally, you mishandled "CacheOperationContext". Please take a look at "GatewayProtectedCacheProxy"
class and usages of "operationContextPerCall" method. What you actually need is to have "opCtx"
field in "GridCacheSetProxy" and pass it into the gate. Please don't forget to restore previous
values on leaving the gate, like, for example, in "GatewayProtectedCacheProxy.CacheOperationGate"
class usages.

Here is the simple reproducer for the problem. It doesn't cover all the aspects but is enough
for the start:
{code:java}
@Test
public void testGet() throws Exception {
    IgniteEx ignite = grid(0);

    IgniteQueue<BinaryObject> queue = ignite.queue("q0", 10, config(false)).withKeepBinary();

    queue.add(sameHashBinObj(ignite, 0));
    queue.add(sameHashBinObj(ignite, 1));

    BinaryObject o0 = queue.poll(); // Ok.

    BinaryObject o1 = GridTestUtils.runAsync((Callable<BinaryObject>)queue::poll).get();
// Fails.
}

private static BinaryObject sameHashBinObj(Ignite ignite, int i) {
    return ignite.binary().toBinary(new SameHashItem(Integer.toString(i)));
}
{code}
You probably noticed that this test operates with "IgniteQueue". It has all the same issues
so they can be reproduced in master. I would appreciate if you created another issue to fix
these bugs in "IgniteQueue" after we finish "IgniteSet" improvements.

 

Please feel free asking me more questions if you have some. Thank you!

> Binary mode for Ignite Set
> --------------------------
>
>                 Key: IGNITE-11397
>                 URL: https://issues.apache.org/jira/browse/IGNITE-11397
>             Project: Ignite
>          Issue Type: New Feature
>          Components: binary, data structures
>            Reporter: Uday Kale
>            Assignee: Uday Kale
>            Priority: Major
>             Fix For: 2.8
>
>          Time Spent: 10m
>  Remaining Estimate: 0h
>
> Add binary mode (withKeepBinary) to Data Structures Set.
> This will allow retrieving values in binary format when needed or when class is not available,
and will allow implementing the structures in other platforms (.NET, C++).



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message