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

Extend java tutorial #2778

Merged

Conversation

Projects
None yet
4 participants
@oleksandr-shabelnyk
Copy link
Contributor

commented Jun 12, 2019

@markus2330 please review my PR

Basics

Check relevant points but please do not remove entries.
Do not describe the purpose of this PR in the PR description but:

  • Short descriptions should be in the release notes (added as entry in
    doc/news/_preparation_next_release.md which contains _(my name)_)
    Please always add something to the the release notes.
  • Longer descriptions should be in documentation or in design decisions.
  • Describe details of how you changed the code in commit messages
    (first line should have module: short statement syntax)
  • References to issues, e.g. close #X, should be in the commit messages.

Checklist

Check relevant points but please do not remove entries.
For docu fixes, spell checking, and similar none of these points below
need to be checked.

  • I added unit tests
  • I ran all tests locally and everything went fine
  • affected documentation is fixed
  • I added code comments, logging, and assertions (see Coding Guidelines)
  • meta data is updated (e.g. README.md of plugins and METADATA.ini)

Review

Reviewers will usually check the following:

Labels

  • Add the "work in progress" label if you do not want the PR to be reviewed yet.
  • Add the "ready to merge" label if the build server is happy and also you
    say that everything is ready to be merged.
@alexsaber

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Can please someone review this PR? Thank you!

@markus2330
Copy link
Contributor

left a comment

Thank you for reminding. Seems like I forgot to submit the review!

//create a key without specifying a name, which is allowed
Key key = Key.create("");
//open KDB with autoclose functionality
try (KDB kdb = KDB.open(key)) {

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

KDB:open is expensive, you should not do this for every key you want to lookup.

This example shows how to read multiple keys. Please read the comments for further clarification.

```java
import org.libelektra.KDB;

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

Better if you put this in a separate java file as an example. In the tutorial you can make a walk through.

<groupId>net.java.dev.jna</groupId>
<artifactId>jna</artifactId>
<version>4.5.0</version>
</dependency>

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

Also here a fully working example would be nice. (examples/external/java would be a good place.)

This comment has been minimized.

Copy link
@alexsaber

alexsaber Jun 14, 2019

Contributor

OK, I will add it.

kdb.get(keySet, Key.create(MOUNT_SPACE));
//fetch value
return keySet.lookup(KEY_PREFIX + keyName).getString();
} catch (KDB.KDBException e) {

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

The lookup should not throw an exception but it might return null, which would cause a NullpointerException when calling getString.

Your tutorial would be much more interesting, if you would also show how a specification with defaults could make sure that this does not happen.

This comment has been minimized.

Copy link
@alexsaber

alexsaber Jun 14, 2019

Contributor

The exception handling is mandatory for kdb.get(keySet, Key.create(MOUNT_SPACE))

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

Yes, but not for the keySet.lookup

import org.libelektra.KeySet;
import java.util.Arrays;
import java.util.Collection;

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

Needed?

This comment has been minimized.

Copy link
@alexsaber

alexsaber Jun 14, 2019

Contributor

I will remove it from the walk through example

};
//read the keys one by one
for (String key : keys) {
properties.put(key, readValue(key));

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

This is weird to copy the config values to properties. This should not be recommended. You can directly lookup your values or you could make a convenience interface to retrieve config values (which internally uses lookup).

This comment has been minimized.

Copy link
@alexsaber

alexsaber Jun 14, 2019

Contributor

OK, I change it

@markus2330 markus2330 requested a review from Piankero Jun 14, 2019

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@Piankero can you also take a look?

@markus2330 markus2330 referenced this pull request Jun 14, 2019

Open

Improving Java Binding #2786

0 of 7 tasks complete

alexsaber added some commits Jun 14, 2019

@alexsaber

This comment has been minimized.

Copy link
Contributor

commented Jun 14, 2019

@markus2330 thank you for your comments. I made all the changes. It is correct now?

@markus2330
Copy link
Contributor

left a comment

did not work for me

@@ -0,0 +1,23 @@
<?xml version="1.0" encoding="UTF-8"?>

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 14, 2019

Contributor

This pom does not build read-keys-example/Main.java for me:

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by com.google.inject.internal.cglib.core.$ReflectUtils$1 (file:/usr/share/maven/lib/guice.jar) to method java.lang.ClassLoader.defineClass(java.lang.String,byte[],int,int,java.security.ProtectionDomain)
WARNING: Please consider reporting this to the maintainers of com.google.inject.internal.cglib.core.$ReflectUtils$1
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release
[INFO] Scanning for projects...
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 0.084 s
[INFO] Finished at: 2019-06-14T21:56:20+02:00
[INFO] Final Memory: 6M/37M
[INFO] ------------------------------------------------------------------------
[ERROR] The goal you specified requires a project to execute but there is no POM in this directory (/home/markus/Projekte/Elektra/libelektra/examples/external/java). Please verify you invoked Maven from the correct directory. -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 
[ERROR] For more information about the errors and possible solutions, please read the following articles:
[ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/MissingProjectException

If I did something wrongly, please write a short README.md to say how to build this example.

This comment has been minimized.

Copy link
@alexsaber

alexsaber Jun 15, 2019

Contributor

There was a misunderstanding. The provided examples were fully working, but no as a project. I bundled them together. Now you should be able to really build it. Please add Elektra jar in the lib dir, like described in the tutorial.

@markus2330
Copy link
Contributor

left a comment

The Java code improved a lot. But I cannot execute the resulting jar.

<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1.0-SNAPSHOT</version>

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 15, 2019

Contributor

I cannot execute the jar file, I get no main manifest attribute, in target/my-app-1.0-SNAPSHOT.jar

@@ -0,0 +1 @@
Copy your libeelektra4j-VERSION.jar file to this directory. The same jar name should be specified in the pom.xml file [here](../pom.xml).

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 15, 2019

Contributor

Please move this file one directory-level up. Otherwise people directly browsing through the folders might not see it.

### Read Multiple Keys From KDB

[This](../../examples/external/java/read-keys-example) example shows how to read multiple keys. It provides comments for further clarification.
If you want to build this project with Maven, please add your jar file like described [here](../../examples/external/java/read-keys-example/lib/README.md).

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 15, 2019

Contributor

(Also fix link here if README.md is moved)

@@ -0,0 +1,67 @@
package com.mycompany.app;

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 15, 2019

Contributor

Please use our domain (org.libelektra) or even no package.


### Read Multiple Keys From KDB

[This](../../examples/external/java/read-keys-example) example shows how to read multiple keys. It provides comments for further clarification.

This comment has been minimized.

Copy link
@markus2330

markus2330 Jun 15, 2019

Contributor

Please describe how to build and execute the jar. Also shortly describe what it does (for people that want to execute it).

@alexsaber

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

The Java code improved a lot. But I cannot execute the resulting jar.

Thank you! I change everything you asked for.
In Intellij I could build and run the project successfully. But not with just maven, as you pointed out correctly. With the new changes, it should be possible. Please build and run it like described in the tutorial.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

Thank you, looks good now!

Very minor points you can ignore:

  • it would be nicer if it would simply start with java -jar target/read-keys-example-jar-with-dependencies.jar
  • target/read-keys-example.jar still does not work, it would be better if it is not produced
  • if the application prints something it would make the reader of the tutorial more confident that it worked
@alexsaber

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

it would be nicer if it would simply start with java -jar target/read-keys-example-jar-with-dependencies.jar
Well, it is possible, but it requires more actions from the developer. Since it is just an example, I think it is better to make it as easy as possible.
target/read-keys-example.jar still does not work, it would be better if it is not produced
Unfortunately, I don't know how to prevent it from building.
if the application prints something it would make the reader of the tutorial more confident that it worked
Ok, I will do it.

@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2019

Thanks for the prints! ❤️

@markus2330 markus2330 merged commit 0ecc6e1 into ElektraInitiative:master Jun 20, 2019

4 of 11 checks passed

mac Task Summary
Details
mac BUILD_FULL:ON BUILD_SHARED:OFF Task Summary
Details
mac KDB_DB_FILE:default.mmap KDB_DB_INIT:elektra.mmap KDB_DEFAULT_STORAGE:mmapstorage Task Summary
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No code changes detected
Details
bsd FreeBSD:freebsd-11-2-release-amd64 Task Summary
Details
bsd FreeBSD:freebsd-12-0-release-amd64 Task Summary
Details
linux Task Summary
Details
mac ASAN_OPTIONS:detect_leaks=1 BINDINGS:cpp ENABLE_ASAN:ON TOOLS:kdb Task Summary
Details
@markus2330

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

Good job!

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.