-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests and resolve issue running SparkGraphComputer on HBase #81
Conversation
<optional>true</optional> | ||
<scope>test</scope> | ||
<exclusions> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
com.lmax.disruptor is only in hbase-server. No need to exclude for hbase-client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Would you combine the commits into one before delivering into master?
@jerryjch – any particular concern with multiple commits? If they're logically distinct, it may be better to keep them separate for readability and future readers, and use a merge commit so that there's only a single point on |
The 5 commits here can be more logically clean and descriptive. |
f615d2a
to
de5af36
Compare
Removed the unnecessary exclusion (good catch) and squashed into a single commit. |
gremlin.graph=org.apache.tinkerpop.gremlin.hadoop.structure.HadoopGraph | ||
gremlin.hadoop.graphInputFormat=org.janusgraph.hadoop.formats.hbase.HBaseInputFormat | ||
gremlin.hadoop.graphOutputFormat=org.apache.hadoop.mapreduce.lib.output.NullOutputFormat | ||
#gremlin.hadoop.graphOutputFormat=org.apache.hadoop.mapreduce.lib.output.SequenceFileOutputFormat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this commented-out line? Is there a use case to prefer this over the NullOutputFormat
on the previous line? Does this help with debugging test failures somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just copied this file from cassandra-read.properties which includes the commented out line (probably for no good reason). Let me know if I should remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, minor questions
gremlin.graph=org.apache.tinkerpop.gremlin.hadoop.structure.HadoopGraph | ||
gremlin.hadoop.graphInputFormat=org.janusgraph.hadoop.formats.hbase.HBaseInputFormat | ||
gremlin.hadoop.graphOutputFormat=org.apache.hadoop.mapreduce.lib.output.NullOutputFormat | ||
#gremlin.hadoop.graphOutputFormat=org.apache.hadoop.mapreduce.lib.output.SequenceFileOutputFormat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets remove it?
assertEquals(numV, (long) t.V().count().next()); | ||
propertiesOnVertex = t.V().valueMap().next(); | ||
valuesOnP = (List)propertiesOnVertex.values().iterator().next(); | ||
assertEquals(numProps, valuesOnP.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it enough to only check the number of properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the version that was in CassandraInputFormatIT and was not modified in this PR. I think changing it would be out of scope here but maybe create an issue for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please create an issue.
|
||
public abstract class AbstractInputFormatIT extends JanusGraphBaseTest { | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit extra line here?
de5af36
to
eaf1bc9
Compare
Should be good now on the requested formatting updates |
@sjudeng – please rebase on |
eaf1bc9
to
c148012
Compare
Done but tests in janusgraph-hadoop which this affects are not currently passing under Travis on master. |
…n HBase Signed-off-by: sjudeng <[email protected]>
c148012
to
e4b57f1
Compare
Add tests and resolve issue running SparkGraphComputer on HBase
Includes refactoring of CassandraInputFormatIT to pull out common tests. Fixes issue in HBaseBinaryInputFormat and HBaseBinaryRecordReader that caused the following error:
Note when merging this PR with #79, the hbase-read.properties test resource in janusgraph-hadoop-core needs to be updated to use JanusGraphKryoRegistrator (see this commit).
References:
thinkaurelius/titan#1268
thinkaurelius/titan#1269