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

Make aws client selectable #16

Merged
merged 20 commits into from
Jun 17, 2024
Merged

Make aws client selectable #16

merged 20 commits into from
Jun 17, 2024

Conversation

egs33
Copy link
Member

@egs33 egs33 commented Jun 14, 2024

Until now, this library has used aws-api.
This PR make available to use other client libraries using protocol.
This version makes it possible to use the AWS SDK.

@egs33 egs33 changed the title Make aws client pluggable Make aws client selectable Jun 14, 2024
@egs33 egs33 force-pushed the make-aws-client-pluggable branch from c7cc3cd to fd52ff5 Compare June 14, 2024 09:55
@egs33 egs33 requested review from liquidz and makinoshi June 17, 2024 00:07
@egs33 egs33 marked this pull request as ready for review June 17, 2024 00:17
Copy link
Member

@makinoshi makinoshi left a comment

Choose a reason for hiding this comment

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

Can you update License section in README.md?

CHANGELOG.md Outdated
@@ -1,6 +1,33 @@
# Change Log

## [Unreleased]
### Breaking Changes
* Add required "client" arguments to start-consumer
Copy link
Member

Choose a reason for hiding this comment

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

Since the 'client' argument already exists, please update the comment to 'Make the client argument mandatory'.

(aws/client {:api :sqs}))
given-client? (some? (:client opts))
num-workers (or (:num-workers opts)
^Consumer [queue-url consume client & [opts]]
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to add an assertion for the client here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Other type checks are done within gluttony.record.consumer/new-consumer, but should I do client here?

https://github.com/toyokumo/gluttony/pull/16/files#diff-2699320efc7348a4f9713eb2ec6eb23b9bb0de16775348543a0b88fadd84df5fR167

Copy link
Member

Choose a reason for hiding this comment

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

I didn't notice that. No action is needed then.

@egs33 egs33 force-pushed the make-aws-client-pluggable branch from cf57eca to 2a3109c Compare June 17, 2024 02:27
Copy link
Member

@liquidz liquidz left a comment

Choose a reason for hiding this comment

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

@egs33 Great work! Overall the PR looks good.
To deploy clojars, license information is required in build.edn now, so please add it.


(testing "Gather every data in order"
;; Add test data
(let [uuid (UUID/randomUUID)]
Copy link
Member

Choose a reason for hiding this comment

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

NIT you can use clojure.core/random-uuid

consume (fn [^Message message respond _]
(log/infof "start to consume:%s" (.body message))
(a/go
;(is (instance? gluttony.record.message.SQSMessage message))
Copy link
Member

Choose a reason for hiding this comment

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

Extra comment?

@@ -0,0 +1,83 @@
(ns gluttony.record.aws-sqs-client
(:require
[clojure.core.async :as as]
Copy link
Member

Choose a reason for hiding this comment

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

NIT In another namespace, clojure.core.async is aliased as a.
We should use same alias name in a project.

consume (fn [message respond _]
(log/infof "start to consume:%s" (:body message))
(a/go
;(is (instance? gluttony.record.message.SQSMessage message))
Copy link
Member

Choose a reason for hiding this comment

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

Extra comment?

build.edn Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Could you add :licenses ?
https://github.com/liquidz/build.edn/blob/main/doc/format/licenses.adoc
Now clojars requires that pom.xml contains license information.
clojars/clojars-web#874

@egs33 egs33 requested a review from liquidz June 17, 2024 04:01
Copy link
Member

@liquidz liquidz left a comment

Choose a reason for hiding this comment

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

@egs33 Thanks for your fixing! LGTM!

@egs33
Copy link
Member Author

egs33 commented Jun 17, 2024

Thank you for reviewing!

@egs33 egs33 merged commit 5d50dc1 into main Jun 17, 2024
1 check passed
@egs33 egs33 deleted the make-aws-client-pluggable branch June 17, 2024 05:01
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.

3 participants