You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@crail.apache.org by patrickstuedi <gi...@git.apache.org> on 2018/02/23 14:51:43 UTC
[GitHub] incubator-crail pull request #6: Applications can now choose on a per node b...
GitHub user patrickstuedi opened a pull request:
https://github.com/apache/incubator-crail/pull/6
Applications can now choose on a per node basis (files, directories,
tables, key/value pairs, etc.) whether the node should be enumerable or
not. If a node is enumerable it will be included in an enumeration
operation of the parent node.
https://issues.apache.org/jira/browse/CRAIL-10
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/patrickstuedi/incubator-crail skipdir
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-crail/pull/6.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #6
----
commit fd750e5b50245ac0987d6c2d7e8505d9fea1e885
Author: Patrick Stuedi <st...@...>
Date: 2018-02-23T14:44:49Z
Applications can now choose on a per node basis (files, directories,
tables, key/value pairs, etc.) whether the node should be enumerable or
not. If a node is enumerable it will be included in an enumeration
operation of the parent node.
https://issues.apache.org/jira/browse/CRAIL-10
----
---
[GitHub] incubator-crail pull request #6: Applications can now choose on a per node b...
Posted by PepperJo <gi...@git.apache.org>.
Github user PepperJo commented on a diff in the pull request:
https://github.com/apache/incubator-crail/pull/6#discussion_r170299288
--- Diff: namenode/src/main/java/org/apache/crail/namenode/TableBlocks.java ---
@@ -34,6 +35,10 @@ public AbstractNode putChild(AbstractNode child) throws Exception {
throw new Exception("Attempt to create key/value pair in container other than a table");
}
- return children.put(child.getComponent(), child);
+ AbstractNode oldNode = children.put(child.getComponent(), child);
+ if (child.isEnumerable()) {
+ child.setDirOffset(dirOffsetCounter.getAndAdd(CrailConstants.DIRECTORY_RECORD));
--- End diff --
Can we have a setDirOffset method in DirectoryBlocks that takes the child as an argument and sets the dir offset accordingly, e.g. something like:
```
void setChildDirOffset(AbstractNode child) {
if (child.isEnumerable()) {
child.setDirOffset(dirOffsetCounter.getAndAdd(CrailConstants.DIRECTORY_RECORD));
}
}
```
---
[GitHub] incubator-crail pull request #6: Applications can now choose on a per node b...
Posted by asqasq <gi...@git.apache.org>.
Github user asqasq commented on a diff in the pull request:
https://github.com/apache/incubator-crail/pull/6#discussion_r170576296
--- Diff: namenode/src/main/java/org/apache/crail/namenode/TableBlocks.java ---
@@ -34,6 +35,10 @@ public AbstractNode putChild(AbstractNode child) throws Exception {
throw new Exception("Attempt to create key/value pair in container other than a table");
}
- return children.put(child.getComponent(), child);
+ AbstractNode oldNode = children.put(child.getComponent(), child);
+ if (child.isEnumerable()) {
+ child.setDirOffset(dirOffsetCounter.getAndAdd(CrailConstants.DIRECTORY_RECORD));
--- End diff --
I would prefer to keep it simple and keep it protected, instead of adding a new method for two reasons:
- A new method would increase code complexity
- The new method would also be inherited, either explicitly "protected" or implicitly "package", so there would not be much of a gain, but comes at the cost of higher code complexity.
---
[GitHub] incubator-crail pull request #6: Applications can now choose on a per node b...
Posted by PepperJo <gi...@git.apache.org>.
Github user PepperJo commented on a diff in the pull request:
https://github.com/apache/incubator-crail/pull/6#discussion_r170300042
--- Diff: rpc/src/main/java/org/apache/crail/rpc/RpcRequestMessage.java ---
@@ -81,24 +87,27 @@ public int write(ByteBuffer buffer) {
buffer.putInt(type.getLabel());
buffer.putInt(storageClass);
buffer.putInt(locationClass);
- written += 12;
+ buffer.putInt(enumerable ? 1 : 0);
+ written += 16;
--- End diff --
Make "16" a final static int constant. Can be also reused in CSIZE above.
---
[GitHub] incubator-crail pull request #6: Applications can now choose on a per node b...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-crail/pull/6
---
[GitHub] incubator-crail pull request #6: Applications can now choose on a per node b...
Posted by PepperJo <gi...@git.apache.org>.
Github user PepperJo commented on a diff in the pull request:
https://github.com/apache/incubator-crail/pull/6#discussion_r170579020
--- Diff: namenode/src/main/java/org/apache/crail/namenode/TableBlocks.java ---
@@ -34,6 +35,10 @@ public AbstractNode putChild(AbstractNode child) throws Exception {
throw new Exception("Attempt to create key/value pair in container other than a table");
}
- return children.put(child.getComponent(), child);
+ AbstractNode oldNode = children.put(child.getComponent(), child);
+ if (child.isEnumerable()) {
+ child.setDirOffset(dirOffsetCounter.getAndAdd(CrailConstants.DIRECTORY_RECORD));
--- End diff --
I disagree, because if you are going to change the logic you have to remember changing it at two places. But I don't feel strong about it. I'm going to approve this.
---
[GitHub] incubator-crail pull request #6: Applications can now choose on a per node b...
Posted by patrickstuedi <gi...@git.apache.org>.
Github user patrickstuedi commented on a diff in the pull request:
https://github.com/apache/incubator-crail/pull/6#discussion_r170575242
--- Diff: namenode/src/main/java/org/apache/crail/namenode/TableBlocks.java ---
@@ -34,6 +35,10 @@ public AbstractNode putChild(AbstractNode child) throws Exception {
throw new Exception("Attempt to create key/value pair in container other than a table");
}
- return children.put(child.getComponent(), child);
+ AbstractNode oldNode = children.put(child.getComponent(), child);
+ if (child.isEnumerable()) {
+ child.setDirOffset(dirOffsetCounter.getAndAdd(CrailConstants.DIRECTORY_RECORD));
--- End diff --
I'm also not a big fan of protected variable, but in this case I prefer the protected over adding another method that has a similar name and similar functionality than setDirOffset that already exists.
---