[GitHub] incubator-crail pull request #6: Applications can now choose on a per node b...
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...
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...
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...
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.
---
[GitHub] incubator-crail pull request #6: Applications can now choose on a per node b...
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...
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...
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 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 ---
