You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tinkerpop.apache.org by dkuppitz <gi...@git.apache.org> on 2015/12/16 20:28:27 UTC
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1026 BVLP should store...
GitHub user dkuppitz opened a pull request:
https://github.com/apache/incubator-tinkerpop/pull/181
TINKERPOP-1026 BVLP should store vertex IDs as String
https://issues.apache.org/jira/browse/TINKERPOP-1026
BLVP (in particular `IncrementalBulkLoader`) now stores the `toString()` version of vertex IDs in the temporary `bulkLoader.vertex.id` property. I also added another `BulkLoader` implementation (`OneTimeBulkLoader`), to a) be able to add more test cases and b) verify that the `toString()` update doesn't interfere with something else.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/apache/incubator-tinkerpop TINKERPOP-1026
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-tinkerpop/pull/181.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 #181
----
commit 8cc12c0bdd9ac96eb0e4e4f7642c81e8055dad8c
Author: Daniel Kuppitz <da...@hotmail.com>
Date: 2015-12-15T21:36:42Z
store the toString() version of vertex IDs and use it for lookups
commit d3ff7533e05d210c5f6bc6ba76d0d2b4944dbde1
Author: Daniel Kuppitz <da...@hotmail.com>
Date: 2015-12-16T17:56:28Z
Added another `BulkLoader` implementation (`OneTimeBulkLoader`) to verify that `id.toString()` vertex lookups still work properly if no temporary property is written to the target graph.
This will not affect the default behavior of `BLVP`, since `IncrementalBulkLoader` will still be used by default, but it allowed to add some more test cases.
commit 2715336642cdb79a12c25e935402954f771dac89
Author: Daniel Kuppitz <da...@hotmail.com>
Date: 2015-12-16T18:14:48Z
updated CHANGELOG
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1026 BVLP should store...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/181#issuecomment-165604630
looks like you angered the travis gods. doesn't look like the error is related to your changes though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1026 BVLP should store...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on a diff in the pull request:
https://github.com/apache/incubator-tinkerpop/pull/181#discussion_r48017013
--- Diff: docs/src/reference/the-graphcomputer.asciidoc ---
@@ -364,10 +364,11 @@ It's recommended to tune this property for the target graph and not use the defa
|`writeGraph(String)` | Sets the path to a `GraphFactory` compatible configuration file for the target graph. | _none_
|========================================
-NOTE: `BulkLoaderVertexProgram` comes with a default `BulkLoader` implementation, namely `IncrementalBulkLoader`. It
-will work for the most use-cases, but has one limitation though: It doesn't support multi-valued properties.
-`IncrementalBulkLoader` will handle every property as a single-valued property. A custom `BulkLoader` implementation
-has to be used if the default behavior is not sufficient.
+NOTE: `BulkLoaderVertexProgram` uses the `IncrementalBulkLoader` by default. The other option is the `OneTimeBulkLoader`,
+which doesn't store any temporary IDs in the `writeGraph` and thus should only be used for initial bulk loads. Both
+implementations should cover the mahority of use-cases, but have a limitation though: They don't support multi-valued
--- End diff --
spelling mistake - should be "majority" :smile:
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1026 BVLP should store...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/181#issuecomment-165551507
I assume that `OneTimeBulkLoader` is something users might use? or was it really just for those two internal use cases you listed there? If it's just internal, perhaps you shouldn't make the class visible to people?
or if it is something that users might use, do you need to update the reference docs to discuss how it is used? maybe some javadocs is enough? both?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1026 BVLP should store...
Posted by twilmes <gi...@git.apache.org>.
Github user twilmes commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/181#issuecomment-165803246
Tests and code look good.
VOTE +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1026 BVLP should store...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/181#issuecomment-165601324
Updated.
* added notes in the reference docs
* made one BLVP sample code snippet use the new `OneTimeBulkLoader`
* verified that `process-docs.sh` still works
* added javadocs for `OneTimeBulkLoader`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1026 BVLP should store...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/181#issuecomment-165760809
`mvn clean install` is good to go and code/docs look good:
VOTE +1
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1026 BVLP should store...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-tinkerpop/pull/181
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1026 BVLP should store...
Posted by dkuppitz <gi...@git.apache.org>.
Github user dkuppitz commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/181#issuecomment-165611868
Hm, worked for me locally. Can we somehow retry Travis builds w/o doing another commit?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-tinkerpop pull request: TINKERPOP-1026 BVLP should store...
Posted by spmallette <gi...@git.apache.org>.
Github user spmallette commented on the pull request:
https://github.com/apache/incubator-tinkerpop/pull/181#issuecomment-165617501
Maybe push an empty commit?
```text
git commit --allow-empty
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---