Skip to content
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

[#737] Add apache accumulo store and test case #759

Merged
merged 3 commits into from May 18, 2018

Conversation

Projects
None yet
5 participants
@chpengzh
Copy link
Contributor

chpengzh commented Mar 8, 2018

a basic implments of accumulo store with accumulo iterator

@s1ck s1ck requested a review from merando Mar 8, 2018

@s1ck

This comment has been minimized.

Copy link
Member

s1ck commented Mar 8, 2018

@merando adding you as reviewer since you have accumulo experience (iirc)

@merando
Copy link
Contributor

merando left a comment

I'm stopping for now. I think there is still work to be done to fit better into gradoop. However, it's a decent start. I will continue my review after the fixes.

* Gradoop Accumulo configuration define
*/
public class GradoopAccumuloConfig extends
GradoopConfig<EPGMGraphHead, EPGMVertex, EPGMEdge> implements Serializable {

This comment has been minimized.

@merando

merando Mar 8, 2018

Contributor

Please extend from the GradoopStoreConfig as it is done in the HBase adapter. Some of the methods below are duplicate code and not necessary since they are implemented in the GradoopFlinkConfig. Hierarchy: GradoopConfig -> GradoopFlinkConfig -> GradoopStoreConfig -> GradoopAccumuloConfig

This comment has been minimized.

@chpengzh

chpengzh Mar 8, 2018

Author Contributor

should I move GradoopStoreConfig from gradoop-hbase to gradoop-flink ?

This comment has been minimized.

@merando

merando Mar 8, 2018

Contributor

I guess this would be ok, since no hbase related code is part of the class

This comment has been minimized.

@chpengzh

chpengzh Mar 16, 2018

Author Contributor

already changed

try {
conn.tableOperations().create(table);
} catch (TableExistsException ignore) {
//ignore if it is exists

This comment has been minimized.

@merando

merando Mar 8, 2018

Contributor

please use the 'tableOperations().exists' method and no empty catch phrases

return config.getGraphHeadTable();
}

@Deprecated

This comment has been minimized.

@merando

merando Mar 8, 2018

Contributor

why are those methods deprecated?

}

@Override
public void setAutoFlush(boolean autoFlush) {

This comment has been minimized.

@merando

merando Mar 8, 2018

Contributor

empty method body

This comment has been minimized.

@chpengzh

chpengzh Mar 9, 2018

Author Contributor

this method is defined by store interface, must be override

/**
* accumulo table store constants
*/
public class AccumuloTables {

This comment has been minimized.

@merando

merando Mar 8, 2018

Contributor

@s1ck do we have coding conventions on classes containing only static string constants? shoudln't this be an ENUM or is this ok for gradoop?

This comment has been minimized.

@s1ck

s1ck Mar 16, 2018

Member

I don't think we have a convention for that. Personally, I don't have a favorite. We use those config classes at other places, too. Enums would require an additional constructor and toString implementation. Interfaces would work too. Whatever you prefer.

row.getGraphs().add(key.getColumnQualifier().toString());
break;
default:
break;

This comment has been minimized.

@merando

merando Mar 8, 2018

Contributor

the default case should never happen, right?

This comment has been minimized.

@chpengzh

chpengzh Mar 29, 2018

Author Contributor

I can't change this, this is required by style-check plugin

[INFO] --- maven-checkstyle-plugin:3.0.0:check (default) @ gradoop-accumulo ---
[INFO] Starting audit...
[ERROR] /Users/chpengzh/Documents/opensource/gradoop/gradoop-accumulo/src/main/java/org/gradoop/common/storage/impl/accumulo/iterator/tserver/GradoopGraphHeadIterator.java:93: switch without "default" clause. [MissingSwitchDefault]
[ERROR] /Users/chpengzh/Documents/opensource/gradoop/gradoop-accumulo/src/main/java/org/gradoop/common/storage/impl/accumulo/iterator/tserver/GradoopVertexIterator.java:93: switch without "default" clause. [MissingSwitchDefault]
[ERROR] /Users/chpengzh/Documents/opensource/gradoop/gradoop-accumulo/src/main/java/org/gradoop/common/storage/impl/accumulo/iterator/tserver/GradoopEdgeIterator.java:93: switch without "default" clause. [MissingSwitchDefault]
Audit done.
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------

pom.xml Outdated
@@ -203,7 +205,16 @@
<configuration>
<source>${project.build.targetJdk}</source>
<target>${project.build.targetJdk}</target>
<!-- configure for flink lambda support -->

This comment has been minimized.

@merando

merando Mar 8, 2018

Contributor

this change should not be made during a PR for Accumulo support. this should be a PR on its own and tested as well. Not sure if we even want to use this right no. I have no experience with it. What's you suggestion @s1ck ?

This comment has been minimized.

@s1ck

s1ck Mar 16, 2018

Member

I agree. @chpengzh is this necessary for this PR?

This comment has been minimized.

@chpengzh

chpengzh Mar 16, 2018

Author Contributor

Seems to be unnessary here.

I found this some days before. I wrote another project which depend on gradoop. I import the whole project (in order to learn api), and decide to implement a transE alg demo. I found there's something wrong with flink lambda support.

<project.build.targetJdk>1.8</project.build.targetJdk>

I'll remove this, or should I will create another issue to describe this after works done?

This comment has been minimized.

@chpengzh

chpengzh Mar 29, 2018

Author Contributor

removed in 7819962

public interface EPGMStore
<G extends EPGMGraphHead, V extends EPGMVertex, E extends EPGMEdge> {
public interface EPGMStore<
C extends GradoopConfig,

This comment has been minimized.

@merando

merando Mar 8, 2018

Contributor

should be GradoopStoreConfig since it is a generic parameter to EPGMStore interface

This comment has been minimized.

@chpengzh

chpengzh Mar 29, 2018

Author Contributor

already changed

/**
* instance operation for java8
*/
public final class Ins {

This comment has been minimized.

@merando

merando Mar 8, 2018

Contributor

This class name is not expressive. It should be clear by the class name what it is meant for. I'm even not sure whether we really need this class.

This comment has been minimized.

@chpengzh

chpengzh Mar 8, 2018

Author Contributor

Sorry, I'll remove it.

BTW, this class means InstanceOperatorUtils, which provides a sets of instance transform with lambda.

This comment has been minimized.

@merando

merando Mar 8, 2018

Contributor

That looks more like a proper name for a utils class

This comment has been minimized.

@chpengzh

chpengzh Mar 8, 2018

Author Contributor

Forget it~ I just forget to remove it

@merando

This comment has been minimized.

Copy link
Contributor

merando commented Mar 8, 2018

how can the build crash after i fixed spelling mistakes in readme and comments?

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Mar 9, 2018

I rewrite the whole accumulo store part to wrap those api define in HBase, and I'll commit it latter after test pass. Thanks for your help and patience @merando

@smee

This comment has been minimized.

Copy link
Contributor

smee commented Mar 9, 2018

@merando The test FoodbrokerTest::testSchema seems to be unstable (see the broken build). I simply restarted the build and now it is successfull.

@s1ck

This comment has been minimized.

Copy link
Member

s1ck commented Mar 9, 2018

@smee @merando I saw that flaky test, too. I'm looking into it.

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Mar 10, 2018

I seems that Persistent* are not suit for accumulo store. So I remind it hbase internal.
But in this way, EPGMStore becomes heavy and with so many template arguments.

I'm thinking about divide it into several interface instead(For example ConfigProvier, GraphWriter, GraphReader)

/**
 * The EPGM store is responsible for writing and reading graph heads, vertices
 * and edges.
 *
 * @param <C> gradoop store config
 * @param <GF> EPGM graph head factory
 * @param <VF> EPGM vertex factory
 * @param <EF> EPGM edge factory
 * @param <IG> EPGM graph head type (input)
 * @param <IV> EPGM vertex type (input)
 * @param <IE> EPGM edge type (input)
 * @param <OG> EPGM graph head type (output)
 * @param <OV> EPGM vertex type (output)
 * @param <OE> EPGM edge type (output)
 */
public interface EPGMStore<
  C extends GradoopStoreConfig<GF, VF, EF>, GF, VF, EF,
  IG extends EPGMGraphHead, IV extends EPGMVertex , IE extends EPGMEdge,
  OG extends EPGMGraphHead, OV extends EPGMVertex, OE extends EPGMEdge> {
  /**
   * Returns the Gradoop configuration associated with that EPGM Store,
   *
   * @return Gradoop Configuration
   */
  C getConfig();
@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Mar 11, 2018

There's something wrong with CI, since I pass the build on my own. https://travis-ci.org/chpengzh/gradoop/jobs/351748770

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Mar 12, 2018

Clear CI cache and It will be ok. @s1ck

"and copy it as accumulo external lib, " + "which locate at $ACCUMULO_HOME/lib/ext");

LOG.info("create mini accumulo cluster instance");
tempDirectory = new File(String.format("/tmp/_accumulo_%d", System.currentTimeMillis()));

This comment has been minimized.

@s1ck
.travis.yml Outdated
@@ -4,6 +4,7 @@ cache:

before_cache:
- rm -rf $HOME/.m2/repository/org/gradoop/
- rm -rf /tmp/_accumulo_*

This comment has been minimized.

@s1ck

s1ck Mar 16, 2018

Member

I don't think you need to clean up temp folders on the CI machine. Or am I wrong?

@s1ck

This comment has been minimized.

Copy link
Member

s1ck commented Mar 16, 2018

I don't think that this should require any manual interaction with CI. Are you sure, it's property set up with the temp folders and all?

@chpengzh chpengzh force-pushed the chpengzh:feature_accumulo_store branch from 7907e5c to 09c130c Mar 16, 2018

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Mar 16, 2018

I revert .travis.yml change and change the category of creating temp file, but there's still something wrong with it

@chpengzh chpengzh force-pushed the chpengzh:feature_accumulo_store branch from 745fbf0 to 50cb48b Mar 17, 2018

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Mar 18, 2018

I think I find the reason, Accumulo MiniCluster doesn't work well on travis env.

I'm trying to fix this

14480 [main] INFO  org.apache.zookeeper.ZooKeeper  - Client environment:java.library.path=/usr/java/packages/lib/amd64:/usr/lib64:/lib64:/lib:/usr/lib
14480 [main] INFO  org.apache.zookeeper.ZooKeeper  - Client environment:java.io.tmpdir=/tmp
14493 [main] INFO  org.apache.zookeeper.ZooKeeper  - Client environment:java.compiler=<NA>
14494 [main] INFO  org.apache.zookeeper.ZooKeeper  - Client environment:os.name=Linux
14494 [main] INFO  org.apache.zookeeper.ZooKeeper  - Client environment:os.arch=amd64
14497 [main] INFO  org.apache.zookeeper.ZooKeeper  - Client environment:os.version=4.14.12-041412-generic
14498 [main] INFO  org.apache.zookeeper.ZooKeeper  - Client environment:user.name=travis
14498 [main] INFO  org.apache.zookeeper.ZooKeeper  - Client environment:user.home=/home/travis
14498 [main] INFO  org.apache.zookeeper.ZooKeeper  - Client environment:user.dir=/home/travis/build/chpengzh/gradoop/gradoop-accumulo
14500 [main] INFO  org.apache.zookeeper.ZooKeeper  - Initiating client connection, connectString=127.0.0.1:37585 sessionTimeout=30000 watcher=org.apache.accumulo.fate.zookeeper.ZooSession$ZooWatcher@70a36a66
14581 [main-SendThread(127.0.0.1:37585)] INFO  org.apache.zookeeper.ClientCnxn  - Opening socket connection to server 127.0.0.1/127.0.0.1:37585. Will not attempt to authenticate using SASL (unknown error)
14589 [main-SendThread(127.0.0.1:37585)] INFO  org.apache.zookeeper.ClientCnxn  - Socket connection established to 127.0.0.1/127.0.0.1:37585, initiating session
14616 [main-SendThread(127.0.0.1:37585)] INFO  org.apache.zookeeper.ClientCnxn  - Session establishment complete on server 127.0.0.1/127.0.0.1:37585, sessionid = 0x16234ea3c2e0004, negotiated timeout = 30000
52213 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
53232 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
54729 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
56230 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
57729 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
59230 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
60729 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
62230 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
63730 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
65230 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
66731 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
68232 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763
69732 [BinMutations 1] WARN  org.apache.accumulo.core.rpc.ThriftUtil  - Failed to open transport to travis-job-chpengzh-gradoop-354771878.travisci.net:45763

@merando merando requested a review from ChrizZz110 Mar 19, 2018

@merando

This comment has been minimized.

Copy link
Contributor

merando commented Mar 26, 2018

@chpengzh hey, any news on that PR? how is it going?

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Mar 26, 2018

@merando I rewrite all your suggested. But there're some problems with accumulo mini cluster. Accumulo Mini Cluster is a test tools to create a minimal accumulo process for test. It is ok when I run mvn test -B on any local file system(both linux and mac, I tried), but it does not work well on travis CI.

First I thought it could be IPv6 problems, but found it wasn't.

I pin my hope to use accumulo docker to rebuild test code, but I have so much work(job...)

I'll continue resolving problem this weekend. Could you please review the code, since all test case can be test pass on local system.

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Mar 29, 2018

Accumulo Mini Cluster require at least 3G RAM.So I change the travis setting by https://docs.travis-ci.com/user/reference/overview/.

But FoodBrokerTest.testSchema(FoodBrokerTest.java:241) break again.

Here's the build from chpengzh/feature_accumulo_store https://travis-ci.org/chpengzh/gradoop/builds/359760250

@s1ck

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Apr 6, 2018

@s1ck @merando

Should I provide benchmark test code for accumulo store? I don't find any code for hbase benchmark test for now.

And should I rebase master, since some changes are made after this pull request started.

@merando

This comment has been minimized.

Copy link
Contributor

merando commented Apr 10, 2018

Yes, please rebase and squash your commits. Afterwards @ChrizZz110 and me will check the PR again.

@chpengzh chpengzh force-pushed the chpengzh:feature_accumulo_store branch from 1c23665 to 0140df0 Apr 10, 2018

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Apr 10, 2018

already rebased but FoodBrokerConfigTest breaks again

Tests run: 7, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 11.085 sec <<< FAILURE! - in org.gradoop.flink.datagen.transactions.foodbroker.FoodBrokerTest
testSchema(org.gradoop.flink.datagen.transactions.foodbroker.FoodBrokerTest)  Time elapsed: 3.899 sec  <<< FAILURE!
java.lang.AssertionError: allocatedTo edges are missing
	at org.gradoop.flink.datagen.transactions.foodbroker.FoodBrokerTest.testSchema(FoodBrokerTest.java:241)
Running org.gradoop.flink.datagen.transactions.foodbroker.FoodBrokerConfigTest
Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.261 sec - in org.gradoop.flink.datagen.transactions.foodbroker.FoodBrokerConfigTest
Running org.gradoop.flink.representation.RepresentationConverterTest
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.222 sec - in org.gradoop.flink.representation.RepresentationConverterTest
Results :
Failed tests: 
  FoodBrokerTest.testSchema:241 allocatedTo edges are missing
Tests run: 1159, Failures: 1, Errors: 0, Skipped: 4

@chpengzh chpengzh force-pushed the chpengzh:feature_accumulo_store branch from 0140df0 to 81f5393 Apr 10, 2018

@merando

This comment has been minimized.

Copy link
Contributor

merando commented Apr 30, 2018

what is the current status? @chpengzh please squash your commits to one (see: https://blog.github.com/2016-04-01-squash-your-commits/ for some advice). After the squash I will take a look.
Any other open questions?

@chpengzh chpengzh force-pushed the chpengzh:feature_accumulo_store branch from 81f5393 to 899c57f Apr 30, 2018

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Apr 30, 2018

I think you mean this? I reset and mix all commit into one, just behind origin master
image

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented Apr 30, 2018

It seems that all problems in accumulo store and unit tests are resolved, since now. Please continue review

@merando
Copy link
Contributor

merando left a comment

I guess we are close to integrating this PR into gradoop cheers great work. please have a look at the minor change requests. How well the accumulo adapter will perform we have to test in real world scenarios i guess.

/**
* json serialize utils
*/
public final class Json {

This comment has been minimized.

@merando

merando Apr 30, 2018

Contributor

the class name should fit its purpose better.

This comment has been minimized.

@chpengzh

chpengzh Apr 30, 2018

Author Contributor

I will change it to JsonUtils

* @param <E> EPGM Element as reading result
* @param <R> ElementRow from iterator rpc result
*/
public class CacheClosableIteratorIterator<E extends EPGMElement, R extends ElementRow> implements

This comment has been minimized.

@merando

merando Apr 30, 2018

Contributor

:) I don't know, but i guess i'm not to much of a fan of "iteratoriterator" do you have an idea for another name fitting the meaning of the class?

This comment has been minimized.

@chpengzh

chpengzh Apr 30, 2018

Author Contributor

CacheClosableIterator ? The second seems to be added by my IDE unconsciously.

pom.xml Outdated
@@ -108,6 +109,7 @@
<license.licenseName>apache_v2</license.licenseName>

<!-- alpha order require -->
<dep.accumulo.version>1.8.1</dep.accumulo.version>

This comment has been minimized.

@merando

merando Apr 30, 2018

Contributor

can you try to upgrade the version to 1.9.x? It was released last week and is afaik only a maintenance release on 1.8.x. Should work seamless.

This comment has been minimized.

@chpengzh

chpengzh May 1, 2018

Author Contributor

Already changed

*/
public abstract class GradoopStoreConfig<G extends EPGMGraphHead, V extends EPGMVertex, E extends EPGMEdge>
extends GradoopFlinkConfig {
public abstract class GradoopStoreConfig<GF, VF, EF> extends GradoopFlinkConfig {

This comment has been minimized.

@merando

merando Apr 30, 2018

Contributor

hm...shouldn't GF, VGF and EF extend the Factories to ensure that factory classes are taken at this point? e.g. if someone implements its own GradoopStoreConfig?

This comment has been minimized.

@chpengzh

chpengzh Apr 30, 2018

Author Contributor

I thought this before, but I found this

PersistentGraphHeadFactory<G extends EPGMGraphHead> extends Serializable
PersistentVertexFactory<V extends EPGMVertex, E extends EPGMEdge> extends Serializable
PersistentEdgeFactory<E extends EPGMEdge, V extends EPGMVertex> extends Serializable

In HBaseConfig, it is not a restriction that GF VF and EF should extends EPGM*Factory, although they are named as *Factory.

This comment has been minimized.

@merando

merando May 2, 2018

Contributor

Hm.... In the original the definition was: GradoopStoreConfig<G extends EPGMGraphHead, V extends EPGMVertex, E extends EPGMEdge>

And G, V, E was used as generic parameter in the Persistent*Factorys. I'm not sure why it was done this way.
Any thoughts on this @s1ck ? What do you think would be the best approach for this PR?

This comment has been minimized.

@merando

merando May 8, 2018

Contributor

@s1ck bump

This comment has been minimized.

@s1ck

s1ck May 8, 2018

Member

iirc, this changed a bit over time. At the moment, one could also use the interfaces directly instead of defining type parameters. Although I'm not sure of the implications, didn't touch that part of the code base for a while. Feel free to refactor as you see fit.

This comment has been minimized.

@merando

merando May 17, 2018

Contributor

ok, I guess this is not to important right now.

@merando

merando approved these changes May 2, 2018

import org.gradoop.common.model.impl.pojo.EdgeFactory;
import org.gradoop.common.model.impl.pojo.GraphHead;

This comment has been minimized.

@ChrizZz110

ChrizZz110 May 2, 2018

Contributor

If there are no content-related changes, files from another module should not be changed. This results in a more time consuming code review process.

This comment has been minimized.

@chpengzh

chpengzh May 2, 2018

Author Contributor

Sorry about that, but I have no other idea to wrap StoreConfig or EPGMStore here. Would you please give some advice?

This comment has been minimized.

@chpengzh

chpengzh May 2, 2018

Author Contributor

Because I have to wrap EPGMStore and GradoopStoreConfig which are defined in hbase module, I have to move those to gradoop-flink. So some changes in gradoop-hbase are necessary, in my opinion.

image

image

This comment has been minimized.

@ChrizZz110

ChrizZz110 May 4, 2018

Contributor

Sorry, you misunderstood my comment. The wrapping is OK and necessary. I meant changing the positions of the import statements are not necessary in such cases. As a reviewer, you see this changes and look for differences in the package names - then you realize that only the position has changed.So everything is fine.

@@ -25,16 +25,16 @@
import org.gradoop.common.config.GradoopHBaseConfig;
import org.gradoop.common.model.api.entities.EPGMEdge;
import org.gradoop.common.model.api.entities.EPGMGraphHead;
import org.gradoop.common.model.api.entities.EPGMVertex;

This comment has been minimized.

@ChrizZz110

ChrizZz110 May 2, 2018

Contributor

If there are no content-related changes, files from another module should not be changed. This results in a more time consuming code review process.

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented May 4, 2018

I see that there's a new patch for FoodBrokerTest. So, should I rebase remote master now?

@s1ck

This comment has been minimized.

Copy link
Member

s1ck commented May 5, 2018

Yes, this should fix the flaky test.

@chpengzh chpengzh force-pushed the chpengzh:feature_accumulo_store branch from fa4dac6 to eda4b38 May 5, 2018

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented May 7, 2018

Done, the head code is now rebase to origin/master.

@merando

This comment has been minimized.

Copy link
Contributor

merando commented May 17, 2018

@ChrizZz110 @chpengzh what is the status? Any changes that still need to be done?

@chpengzh

This comment has been minimized.

Copy link
Contributor Author

chpengzh commented May 18, 2018

@ChrizZz110

This comment has been minimized.

Copy link
Contributor

ChrizZz110 commented May 18, 2018

@merando Sorry I found no time for continuing the review of this PR. But I blocked two hours this afternoon to finish my review.

@Suite.SuiteClasses({
AccumuloGraphStoreTest.class, AccumuloDataSinkSourceTest.class,
})
public class AccumuloTestSuit {

This comment has been minimized.

@ChrizZz110

ChrizZz110 May 18, 2018

Contributor

not AccumuloTestSuite?

@ChrizZz110

This comment has been minimized.

Copy link
Contributor

ChrizZz110 commented May 18, 2018

For me, everything is fine with this PR. Unfortunately I had no time to test it with a real accumulo instance to write and read large graphs.

@chpengzh chpengzh force-pushed the chpengzh:feature_accumulo_store branch from eda4b38 to 437ce3b May 18, 2018

@merando merando merged commit 0b27a64 into dbs-leipzig:master May 18, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@merando

This comment has been minimized.

Copy link
Contributor

merando commented May 18, 2018

Ok, merged! Thanks for your hard work @chpengzh !
Further contributions are very welcome.

0x002A pushed a commit to ChrizZz110/gradoop that referenced this pull request Feb 19, 2019

[dbs-leipzig#737] Add apache accumulo store and test case (dbs-leipzi…
…g#759)

* add accumulo store implements and test unit

* update readme, rename confusing class name

* fix spelling mistake of test suite name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.