nervous-systems / hildebrand

Asynchronous DynamoDB client for Clojure & Clojurescript/Node
The Unlicense
66 stars 10 forks source link

WIP #24

Closed ChelseyM closed 7 years ago

ChelseyM commented 7 years ago

Fixes #16

ChelseyM commented 7 years ago

Not quite there yet

moea commented 7 years ago

@ChelseyM Thanks for tackling this & apologies for the byzantine key mapping stuff - I wrote it some time ago. Are you looking for feedback or you want me to take a look when it's done?

ChelseyM commented 7 years ago

@moea Haha no worries. Let me take one more stab at it, as it currently spits out

{:table-name ({:put {:item {:number {:N 16.0}}})}

instead of

{:put {:table-name ({:name "foo"})}}

this will also only work with batch-write-item, and I originally ran into the problem with batching-puts, so I'd love to try to think of a solution for that.

Will check back in after my next commit + then feedback would be greatly appreciated!

ChelseyM commented 7 years ago

Ok, so I came back to this many months later 😅 It now restructures the response from

{:unprocessed-items
 {:table-name
  [{:put-request
    {:item
     {:k-1 {:N "16.0"},
      :k-2 {:BOOL "true"},
      :k-3 {:NULL "true"},
      :k-4 {:S "7VNbgwdWAPzjVHWDw7DooP"}}}}
   {:put-request
    {:item
     {:k-1 {:N "0.010766208171844482"},
      :k-2 {:BOOL "false"},
      :k-3 {:NULL "true"},
      :k-4 {:S "mAX15D3N"}}}}
   {:delete-request
    {:key {:k-1 {:L [{:N "292.0131530761719"} {:N "7.0"}]}}}}],
  :table-name-2
  [{:put-request
    {:item
     {:k-1 {:N "16.0"},
      :k-2 {:BOOL "true"},
      :k-3 {:NULL "true"},
      :k-4 {:S "7VNbgwdWAPzjVHWDw7DooP"}}}}
   {:delete-request
    {:key {:k-1 {:L [{:N "292.0131530761719"} {:N "7.0"}]}}}}]}}

to

{:unprocessed
 {:put
  {:table-name
   ({:k-1 0.010766208171844482M, :k-2 true, :k-3 nil, :k-4 "mAX15D3N"}
    {:k-1 16.0M, :k-2 true, :k-3 nil, :k-4 "7VNbgwdWAPzjVHWDw7DooP"}),
   :table-name-2
   ({:k-1 16.0M, :k-2 true, :k-3 nil, :k-4 "7VNbgwdWAPzjVHWDw7DooP"})},
  :delete
  {:table-name ({:k-1 [292.0131530761719M 7.0M]}),
   :table-name-2 ({:k-1 [292.0131530761719M 7.0M]})}}}

Feedback would be greatly appreciated :)

Also I realize the tests fail, but I will look into that in a moment

moea commented 7 years ago

Hey, thanks for getting to this! I'll be able to provide more practical feedback once I get a chance to run it, but I'll leave some comments in-line.

moea commented 7 years ago

I understand that approach, but we'd want to change it in other places, also, for consistency, like batch-get-item. It might make sense to confine this PR to the formatting, and handle errors in another one?

My original thinking was to try and make it as conspicuous as possible that the request wasn't entirely processed, while still making the data available in the error map.

On 22 Aug 2017 5:38 pm, "Chelsey Mitchell" notifications@github.com wrote:

@ChelseyM commented on this pull request.

In src/hildebrand/internal/response.cljc https://github.com/nervous-systems/hildebrand/pull/24#discussion_r134536058 :

@@ -163,8 +175,13 @@

(defmethod restructure-response* :hildebrand.response/batch-write-item [_ {:keys [unprocessed] :as resp}]

  • (or (maybe-unprocessed-error unprocessed)
  • (with-meta {} resp)))
  • (let [result (with-meta

Well I guess I was basing this thinking off how failures are routine/expected with dynamodb, and you're expected to retry until the items get processed.

The individual PutItem and DeleteItem operations specified in BatchWriteItem are atomic; however BatchWriteItem as a whole is not. If any requested operations fail because the table's provisioned throughput is exceeded or an internal processing failure occurs, the failed operations are returned in the UnprocessedItems response parameter. You can investigate and optionally resend the requests. Typically, you would call BatchWriteItem in a loop. Each iteration would check for unprocessed items and submit a new BatchWriteItem request with those unprocessed items until all items have been processed.

It would also be useful in the channeled version of this to return the unprocessed items back on a channel to make retrying easier versus treating it as an error. Do you think it should be handled differently here?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nervous-systems/hildebrand/pull/24#discussion_r134536058, or mute the thread https://github.com/notifications/unsubscribe-auth/ABYuK3ITdPrJk_9yAO3CEzBie3YgMoOvks5sawQYgaJpZM4Nbs4E .

moea commented 7 years ago

Great, I'll merge this and push a snapshot tomorrow. Thanks!

moea commented 7 years ago

@ChelseyM Just to confirm, are you happy with where the branch is?

ChelseyM commented 7 years ago

Yes :) as long as you are!

On Sun 24. Sep 2017 at 17:20, Moe Aboulkheir notifications@github.com wrote:

@ChelseyM https://github.com/chelseym Just to confirm, are you happy with where the branch is?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nervous-systems/hildebrand/pull/24#issuecomment-331740948, or mute the thread https://github.com/notifications/unsubscribe-auth/AGpR4Sti3zW4M0kTzShog2dod5jMGEdgks5slseGgaJpZM4Nbs4E .

ChelseyM commented 7 years ago

Build is failing though, did I cause that?

On Sun 24. Sep 2017 at 17:51, Chelsey Mitchell m.chelsey@gmail.com wrote:

Yes :) as long as you are!

On Sun 24. Sep 2017 at 17:20, Moe Aboulkheir notifications@github.com wrote:

@ChelseyM https://github.com/chelseym Just to confirm, are you happy with where the branch is?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nervous-systems/hildebrand/pull/24#issuecomment-331740948, or mute the thread https://github.com/notifications/unsubscribe-auth/AGpR4Sti3zW4M0kTzShog2dod5jMGEdgks5slseGgaJpZM4Nbs4E .

moea commented 7 years ago

There's an unrelated Travis configuration issue - it's not getting as far as running the tests. I'll fix it and run the tests locally before releasing anything.

ChelseyM commented 7 years ago

Cool!

On Sun 24. Sep 2017 at 18:27, Moe Aboulkheir notifications@github.com wrote:

There's an unrelated Travis configuration issue - it's not getting as far as running the tests. I'll fix it and run the tests locally before releasing anything.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/nervous-systems/hildebrand/pull/24#issuecomment-331744641, or mute the thread https://github.com/notifications/unsubscribe-auth/AGpR4QezysHlSLzxSlHD56gYHDzcWq3iks5sltdsgaJpZM4Nbs4E .

moea commented 7 years ago

@ChelseyM 0.4.6-SNAPSHOT deployed to clojars, lmk if you have any issues.