Closed dgcaron closed 6 years ago
Thanks for the pull request!! To ensure quality review, Couchbase employs a code review system based on Gerrit to manage the workflow of changes in addition to tracking our contributor agreements.
Your changes (commit: 3dba57267bce5e86e3f729fd52467de26d58bab8) have been pushed to the Couchbase Review Site: http://review.couchbase.org/94119
::SDKBOT/PR:created
Hey @daschl , when you have a chance, can you take a look at this and see if this is how you'd address the timeout issues seen by the customer?
Did a quick review, I think the subdoc section can be simplified (and the test change is confusing me a bit)
Hi @dgcaron,
We're standing by, waiting for a code update. Let us know if you need any help with Gerrit. Please ping @daschl if his remarks need clarification or elaboration.
Thanks, David
Hi David,
We are running some more tests with the adjusted code, I expect to update the PR end of week. My apologies for the delay!
On Tue, 5 Jun 2018, 01:13 dnault, notifications@github.com wrote:
Hi @dgcaron https://github.com/dgcaron,
We're standing by, waiting for a code update. Let us know if you need any help with Gerrit. Please ping @daschl https://github.com/daschl if his remarks need clarification or elaboration.
Thanks, David
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/couchbase/kafka-connect-couchbase/pull/8#issuecomment-394529270, or mute the thread https://github.com/notifications/unsubscribe-auth/AJT_IFmlnKgTEhMOdw4odwIDEqHbTbk6ks5t5b8IgaJpZM4T89Vd .
Your changes (commit: 115a93bbe0045f3e66267c742b36b91ce30cec54) have been pushed to the Couchbase Review Site: http://review.couchbase.org/94119
Note: As your pull request contains multiple commits, we have performed an automatic squash of these commits into a single change-set. If this is not the desired behaviour, please consider submitting a pull request per discreet feature.
::SDKBOT/PR:updated
Your changes (commit: 25c73dfdb95c3ab63e6125737fca0108c7b6e02d) have been pushed to the Couchbase Review Site: http://review.couchbase.org/94119
Note: As your pull request contains multiple commits, we have performed an automatic squash of these commits into a single change-set. If this is not the desired behaviour, please consider submitting a pull request per discreet feature.
::SDKBOT/PR:updated
Your changes (commit: 3d4272b3da3dc5c5baa3acd4539c6ae670578ee7) have been pushed to the Couchbase Review Site: http://review.couchbase.org/94119
Note: As your pull request contains multiple commits, we have performed an automatic squash of these commits into a single change-set. If this is not the desired behaviour, please consider submitting a pull request per discreet feature.
::SDKBOT/PR:updated
hmm this last commit should have been a new pull request.. i didn't amend the commit that is connected to this pull request. do you want me to revert or keep this change as part of this PR?
Looks like a sizable change. Probably better to revert it for now and handle it in a separate review.
i have reset the master to point to the previous commit, anything i need to do here to fix this?
The PR in Gerrit shows modifications in several unrelated files; I think this just means your changes need to be rebased on top the the latest master.
Hmm I did rebase it on your master.. Sorry David, still learning this OSS stuff
Hi Didier,
Would you be open to splitting this PR into a few smaller ones? In particular, it would be nice to start with a PR that has only the DocumentIdExtractor
changes. This would make the review more manageable, and would let us start with a clean slate (I'm not sure how to fix the current PR -- here's what it looks like in Gerrit).
Let me know how you want to proceed.
Since you mentioned you're relatively new to collaborating on OSS, is it okay if I offer some workflow advice? To make sure you don't lose your existing changes, I'd recommend first creating a branch off your master:
git checkout master
git checkout -b original-work-for-reference
git push --set-upstream origin original-work-for-reference
Then reset your master to be exactly the same as the upstream repo:
git checkout master
git fetch upstream
git reset --hard upstream/master
git push -f
Replace "upstream" with whatever name you used when adding the Couchbase repo as a remote. (The -f
option to push
does a "force push", which is required because you're rewriting history.)
Then (here's the really important part!) create a feature branch off master, and make your modifications in that branch (create a separate branch for each feature you're working on):
git checkout master
git checkout -b my-feature-branch
git push --set-upstream origin my-feature-branch
Commit and push your changes on that feature branch, then create the pull request from that feature branch (instead of master). This helps Gerrit understand that you only want to apply the changes that are in that branch.
Periodically, and especially right before you create the PR, you'll want to rebase your feature branch on top of the latest upstream changes:
git fetch upstream
git checkout my-feature-branch
git rebase upstream/master
git push -f
Rebasing rewrites history, so the -f
is required to force push. If there has been a lot of activity in the upstream repo, there might be conflicts during the rebase. If so, take a look at resolving merge conflicts after a git rebase.
Once your PR has been merged, you can delete the feature branch and create a new one for your next change.
Thanks, David
Your changes (commit: 115a93bbe0045f3e66267c742b36b91ce30cec54) have been pushed to the Couchbase Review Site: http://review.couchbase.org/94119
Note: As your pull request contains multiple commits, we have performed an automatic squash of these commits into a single change-set. If this is not the desired behaviour, please consider submitting a pull request per discreet feature.
::SDKBOT/PR:updated
closing the PR to split up the PR in several small ones
We noticed when running the connector with some load the connector started to throw timeout exceptions. this occured when documents needed to be created after an update failed (returned 0 rows). there were blocking calls in the retry mechanism that according to this thread (https://forums.couchbase.com/t/couchbase-server-randomly-disconnects-kv-endpoints-keepalive-not-working/9607/6) can cause timeout exceptions. this PR is an attempt to get the retry mechanism better without any blocking calls.