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

Feature: Datomic Client #505

Closed
wants to merge 5 commits into from

Conversation

bhurlow
Copy link

@bhurlow bhurlow commented Jul 10, 2020

This pull request introduces a feature flag which compiles the Datomic pro client library (version 0.9.57) into babashka. To get the client working I needed to do the following:

  • require the transitive namespaces needed by the client library which are dynamically loaded at runtime
  • allow the .pem certificated used as a resource in the client jar to be copied into the graal build via resources-config.json
  • add several reflected classes to the classes list
  • define the java.library.path system property which Datomic client uses to look up certificates. This piece may need some more attention.

Questions

  1. The client test requires a running peer server with the expected data. Is an integration test in CI going to be too heavy?

  2. Does this require permission from the Datomic team? I feel it's a gray area because the jar asset is fully public in maven and doesn't require a license key, but of course we should honor the Datomic EULA.

Misc Comments

  • I have not tested the async api, only the sync one
  • I have not exhaustively tested every client function, there could be more transitive deps which are dynamically required that I didn't catch.

Initial performance looks pretty promising, below is the result of running the test with a peer server on my macbook. The whole startup and several tests takes ~15ms

time ./bb --file client-test.clj
- transact

{temp-foo 17592206775568}


- query

{:user/username bingo, :user/firstname Brian (Bingo)}


- tx-range

#uuid "5b7ad9e2-d975-4a1b-a3cc-0e8463fe0ca5"
#uuid "5b7ad9ec-8004-435b-876c-139cfaa3a323"
#uuid "5b7ad9ec-bef3-4170-8b3f-d0a315cf7896"
#uuid "5b7ad9ec-9ca5-45bb-b94f-c27c3279f397"
#uuid "5b7ad9ec-6f91-49ec-a993-115614a97c2f"
#uuid "5b7ad9ec-4378-48be-9649-d053ce1c0861"
#uuid "5b7ad9ec-83bc-4dec-8c30-4f7364ee6cb2"
#uuid "5b7ad9ec-be4c-4274-861b-15d4ec872599"
#uuid "5b7ad9ec-3cd6-448d-ae8a-3506f5514287"
#uuid "5b7ad9ec-ef6a-462b-b883-eca5738017a0"
nil


- index range

(#datom[17592186189310 270 0c125946 13194139678205 true] #datom[17592186150945 270 Education-Network 13194139639840 true] #datom[17592186150954 270 Network-Interface 13194139639849 true] #datom[17592187041685 270 acs 13194140530580 true] #datom[17592186706745 270 advanced-education-programs 13194140195640 true] #datom[17592186707270 270 agriculture 13194140196165 true] #datom[17592186538838 270 agroecology 13194140027733 true] #datom[17592189283410 270 air-university 13194142772305 true] #datom[17592186707111 270 allied-health 13194140196006 true] #datom[17592186139214 270 alumni 13194139635042 true])


0.15 real         0.05 user         0.03 sys

Open to any and all feedback, our team is very excited to use this for db scripting

@bhurlow
Copy link
Author

bhurlow commented Jul 10, 2020

looks like the branch requires java.home to be set at runtime

@@ -21,7 +22,8 @@
[cheshire "5.10.0"]
[fipp "0.6.22"]
[nrepl/bencode "1.1.0"]
[borkdude/sci.impl.reflector "0.0.1-java11"]]
[borkdude/sci.impl.reflector "0.0.1-java11"]
[com.datomic/client-pro "0.9.57"]]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dependency needs to be hidden behind a profile like :feature/datomic-client

@@ -0,0 +1,62 @@
(ns client-test
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script probably needs to be implemented as a proper test. See the postgres tests how to execute a test based on a feature flag.

@@ -0,0 +1,10 @@
{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We manage resources dynamically in script/compile and script/compile.bat based on feature flags.

@@ -140,6 +140,17 @@
;; java.net.URL, see below
java.net.URLEncoder
java.net.URLDecoder
;;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of these classes only need to be brought into the reflection config when the feature flag is enabled.

@borkdude
Copy link
Collaborator

borkdude commented Jul 11, 2020

Re: The client test requires a running peer server with the expected data. Is an integration test in CI going to be too heavy?

The postgresql feature is also optional and I don't run this test in CI since babashka is only compiled with the default feature flags in CI. If your company is going to use the feature-flagged build of babashka, I recommend running the tests in your own CI. I do welcome an integration test that can be run when the feature flag is enabled. Maybe the new Datomic dev-local thing can be used for this? https://docs.datomic.com/cloud/dev-local.html
See the postgresql tests for an example of integration tests that aren't run in CI but are run when you enable the feature flag.

Re: Does this require permission from the Datomic team? I feel it's a gray area because the jar asset is fully public in maven and doesn't require a license key, but of course we should honor the Datomic EULA.

What does this refer to?

@borkdude
Copy link
Collaborator

borkdude commented Jul 11, 2020

Re java.library.path: you can also set the library path dynamically in a babashka program: (System/setProperty ...). Maybe that helps? Note that it works at runtime for a GraalVM binary, but not for a JVM, then it has to be set using the -D flag.

@borkdude borkdude marked this pull request as draft July 11, 2020 07:49
@borkdude
Copy link
Collaborator

borkdude commented Jul 11, 2020

Btw, another cool idea is to make a pod out of this, similar to babashka-sql-pods. See https://github.com/babashka/babashka.pods and https://github.com/babashka/babashka-sql-pods. That may be something to look at after this PR gets merged.

@borkdude
Copy link
Collaborator

Thanks for showing some promising results. I will close this for now because of lack of activity, but will re-open when needed.

@borkdude borkdude closed this Dec 24, 2021
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

Successfully merging this pull request may close these issues.

None yet

2 participants