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

Add Clojure duct framework #3874

Merged
merged 9 commits into from Jul 4, 2018
Merged

Conversation

agrison
Copy link
Contributor

@agrison agrison commented Jun 20, 2018

Add implementation for the Clojure's duct framework.

@@ -0,0 +1,4 @@
(ns hello.handler.example-test
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove this test file? I see it is basically an empty placeholder, but even if it wasn't empty, we still don't want to maintain framework-specific functional tests in this repository.

I made a similar request in another PR recently with a longer explanation: #3895 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the test file which was generated by the lein template.


(defn- query [db]
(first
(jdbc/query db ["select * from world where id = ?" (inc (rand-int 4))])))
Copy link
Contributor

Choose a reason for hiding this comment

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

It's supposed to be a random number between 1 and 10,000 (inclusive on both ends), so I think this should be (rand-int 10000) instead of (rand-int 4).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, I only had some rows in local, before I configured Docker on my machine 👍

(jdbc/query db ["select * from world where id = ?" (inc (rand-int 4))])))

(defn- mongo-query [db]
(dissoc (mc/find-one-as-map db "world" {:id 5}) :_id))
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like it's hardcoded to find the world with id=5. It should be a randomly-generated id just like in the SQL version.

"language": "Clojure",
"flavor": "None",
"orm": "Raw",
"platform": "Servlet",
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking - is "platform": "Servlet" correct? I don't see a servlet container such as Resin or Tomcat being used in the dockerfiles to launch the web application, so I'm wondering if this part is intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, the platform for the default, mongodb and httpkit is supposed to be Ring. I also updated the other implementations like aleph -> Netty, immutant -> Undertow.

; :duct.logger.timbre/brief #ig/ref :duct.logger.timbre/brief}}}

:duct.database.mongodb/monger
{:host "localhost" :port 27017}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this file used by the implementation that gets benchmarked? If not, if it's a config file only used when launching the application directly rather than running it through the TFB toolset, then please remove it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same with any other files like this (I see a few others that are suspicious). We don't want any source code that's not being used in the benchmarks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These files are useful for local development (in order to test from the REPL), I'll remove them from the repository and keep them locally 👍

@agrison
Copy link
Contributor Author

agrison commented Jul 4, 2018

I will investigate why it does not start anymore while on the train 😄

@michaelhixson michaelhixson merged commit 82eadc9 into TechEmpower:master Jul 4, 2018
roberthusak pushed a commit to roberthusak/FrameworkBenchmarks that referenced this pull request Nov 6, 2018
* Add Clojure Duct framework

* Clean up comments

* Disable logging

* Remove useless testing file.

* Update invalid rand number.

* Update platform in benchmark_config

* Remove localhost config.

* Remove development platform config

* Update docker files.
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