[jira] [Comment Edited] (IGNITE-642) Implement IgniteReentrantLock data structure

["Yakov Zhdanov (JIRA)" <ji...@apache.org>]

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

Yakov Zhdanov edited comment on IGNITE-642 at 3/6/16 9:21 AM:
--------------------------------------------------------------

Vlad,

Thank you for your efforts! I think we are very close to the final step.

Here are my comments. I think I will review the code one more time and provide more comments if any. 

1. classnames.properties is changed in the PR. Can you please ask Anton Vinogradov what is the process for this and if we still use this file? Is it autogenerated or not? I searched our wiki for "classnames.properties" but found nothing.
2. trivial compilation error - org/apache/ignite/internal/processors/datastructures/DataStructuresProcessor.java:1435 - "broken" variable is not final. Vlad, are you sure this is the final version?
3. java 7 generics warning - GridCacheReentrantLockImpl.java:497 + code style  - I will resolve both on PR merge
4. IgniteReentrantLock interface docs&methods seems to be copied from JDK which is not correct in my view. It contains, for example, a lot of "new ReentrantLock();" mentions.
5. I think IgniteReentrantLock should:
- rename to IgniteLock
- extend SDK lock interface and override methods for providing proper documentation.
- newCondition() should throw unsupported operation exception and should send user to use getOrCreateCondition(String name);
- IgniteCondition should also extend SDK condition and provide proper documentation

6. org.apache.ignite.internal.processors.datastructures.GridCacheReentrantLockImpl.Sync#failedNodes - seems to only grow which is strange and may lead to OOME (of course in theory, but still). It seems you can remove this collection. You can check if node is failed or not by checking it with discovery - kernalContext.discovery().node(id); Will it work?


was (Author: yzhdanov):
Vlad,

Thank you for your efforts! I think we are very close to the final step.

Here are my comments. I think I will review the code one more time and provide more comments if any. 

1. classnames.properties is changed in the PR. Can you please ask Anton Vinogradov what is the process for this and if we still use this file? Is it autogenerated or not? I searched our wiki for "classnames.properties" but found nothing.
2. trivial compilation error - org/apache/ignite/internal/processors/datastructures/DataStructuresProcessor.java:1435 - "broken" variable is not final. Vlad, are you sure this is the final version?
3. java 7 generics warning - GridCacheReentrantLockImpl.java:497 + code style  - I will resolve both on PR merge
4. IgniteReentrantLock interface docs&methods seems to be copied from JDK which is not correct in my view. It contains, for example, a lot of "new ReentrantLock();" mentions.
5. I think IgniteReentrantLock should:
- rename to IgniteLock
- extend SDK lock interface and override methods for providing proper documentation.
- newCondition() should throw unsupported operation exception and should send user to use getOrCreateCondition(String name);
- IgniteCondition should also extend SDK condition and provide proper documentation
6. org.apache.ignite.internal.processors.datastructures.GridCacheReentrantLockImpl.Sync#failedNodes - seems to only grow which is strange and may lead to OOME (of course in theory, but still). It seems you can remove this collection. You can check if node is failed or not by checking it with discovery - kernalContext.discovery().node(id); Will it work?

> Implement IgniteReentrantLock data structure
> --------------------------------------------
>
>                 Key: IGNITE-642
>                 URL: https://issues.apache.org/jira/browse/IGNITE-642
>             Project: Ignite
>          Issue Type: Sub-task
>          Components: data structures
>            Reporter: Dmitriy Setrakyan
>            Assignee: Vladisav Jelisavcic
>
> We need to add {{IgniteReentrantLock}} data structure in addition to other data structures provided by Ignite. {{IgniteReentrantLock}} should have similar API to {{java.util.concurrent.locks.ReentrantLock}} class in JDK.
> As an example, you can see how [IgniteCountDownLatch|https://github.com/apache/incubator-ignite/blob/master/modules/core/src/main/java/org/apache/ignite/IgniteCountDownLatch.java] is implemented in [GridCacheCountDownLatchImpl|https://github.com/apache/incubator-ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/processors/datastructures/GridCacheCountDownLatchImpl.java] class.
> In general we need to have an entity in ATOMIC cache storing lock-owner identifier together with a queue of waiters.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)