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

[ignite, ignite-sql] clean up for apache ignite clients #1183

Closed
busbey opened this issue Jul 9, 2018 · 5 comments
Closed

[ignite, ignite-sql] clean up for apache ignite clients #1183

busbey opened this issue Jul 9, 2018 · 5 comments

Comments

@busbey
Copy link
Collaborator

busbey commented Jul 9, 2018

After the merge of #1118 here are some things to take care of in the Apache Ignite (incubating) clients, preferably before the next release.

Files missing a proper license header:

  • ignite/src/main/java/com/yahoo/ycsb/db/ignite/IgniteAbstractClient.java
  • ignite/src/main/resources/log4j2.xml
  • ignite/src/test/java/com/yahoo/ycsb/db/ignite/IgniteClientCommonTest.java

Files that still have @author tags that need to be removed:

  • ignite/src/main/java/com/yahoo/ycsb/db/ignite/IgniteAbstractClient.java
  • ignite/src/main/java/com/yahoo/ycsb/db/ignite/IgniteClient.java
  • ignite/src/main/java/com/yahoo/ycsb/db/ignite/IgniteSqlClient.java

Need to confirm if copyright year is correct:

  • ignite/src/test/java/com/yahoo/ycsb/db/ignite/IgniteClientTest.java
  • ignite/src/test/java/com/yahoo/ycsb/db/ignite/IgniteSqlClientTest.java

from ignite/README.md

+### 2. Start Apache Ignite
+1.1 Download latest binary [Apache Ignite release](https://ignite.apache.org/download.cgi#binaries)
+
+1.2 Start ignite nodes using apache-ignite-fabric-2.5.0-bin/bin/**ignite.sh** ignite.xml
+
+1.3 Copy YCSB/ignite/target/ignite-binding-0.15.0-SNAPSHOT.jar to apache-ignite-fabric-2.5.0-bin/libs
+

Can we avoid copying things into the ignite installation?

+Run the workload test with IgniteClient:
...
+Run the workload test with IgniteSqlClient:
+

we should avoid referencing the internal classes and just use the binding names "ignite" and "ignite-sql".

from ignite/pom.xml

+  <repositories>
+    <repository>
+      <id>GridGain External Repository</id>
+      <url>http://www.gridgainsystems.com/nexus/content/repositories/external</url>
+    </repository>
+  </repositories>

I don't see any dependencies that specifically need this repository. Can we remove it?

from ignite/src/main/java/com/yahoo/ycsb/db/ignite/IgniteClient.java

+  @Override
+  public Status scan(String table, String startkey, int recordcount,
+                     Set<String> fields, Vector<HashMap<String, ByteIterator>> result) {
+    throw new UnsupportedOperationException("Scan method isn't implemented");
+  }

This should return Status.NOT_IMPLEMENTED

from ignite/src/main/java/com/yahoo/ycsb/db/ignite/IgniteSqlClient.java

+  /**
+      Unsupported operation.
+   */
+  @Override
+  public Status scan(String table, String startkey, int recordcount,
+                     Set<String> fields, Vector<HashMap<String, ByteIterator>> result) {
+    try {
+      return Status.OK;
+
+    } catch (Exception e) {
+      log.error(String.format("Error scanning with startkey: %s", startkey), e);
+
+      return Status.ERROR;
+    }
+
+  }

This should return Status.NOT_IMPLEMENTED

@busbey
Copy link
Collaborator Author

busbey commented Jul 14, 2018

any chance you'd have time to work these things in the short term @isuntsov-gridgain ? I'd like to start the next release soon, but need these cleaned up first.

@isuntsov-gridgain
Copy link
Contributor

isuntsov-gridgain commented Jul 14, 2018 via email

@busbey
Copy link
Collaborator Author

busbey commented Jul 14, 2018

awesome, thanks!

@isuntsov-gridgain
Copy link
Contributor

@busbey please take a look at #1189

@isuntsov-gridgain
Copy link
Contributor

Done #1189

busbey added a commit to busbey/YCSB that referenced this issue Jul 27, 2018
…che Ignite client (addendum)

* Ignite bindings should always return Status.NOT_IMPLEMENTED for scans.
* rename base test class so that JUnit won't try to run the scan test without an implementation
* remove unneeded additional repository definition

Closes brianfrankcooper#1183
@busbey busbey closed this as completed in 7e19b75 Jul 27, 2018
busbey added a commit that referenced this issue Jul 27, 2018
…che Ignite client (addendum)

* Ignite bindings should always return Status.NOT_IMPLEMENTED for scans.
* rename base test class so that JUnit won't try to run the scan test without an implementation
* remove unneeded additional repository definition

Closes #1183
@busbey busbey mentioned this issue Jul 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants