[
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)
