convert erlfdb_error 2101 to transaction_too_large #3215

jiangphcn · 2020-10-13T03:02:44Z

Overview

If transaction exceeds byte limit using the _bulk_docs endpoint, the {u'reason': u'2101', u'ref': 3853288219, u'error': u'erlfdb_error'} with 500 error code was returned. This is user-friendly. Using this PR is to provide meaningful error message to users.

Testing recommendations

The test python code:

import requests, json, random, string, os
print(requests.post(
    "http:https://127.0.0.1:15984/foo/_bulk_docs"
    , json={
        "docs":
            [
                {"foo": "".join(random.choice(string.ascii_lowercase) for x in range(16000000))}
            ]}
    , auth=('foo', 'bar')
).json())

Original response:

{u'reason': u'2101', u'ref': 3853288219, u'error': u'erlfdb_error'}

New response:

{u'reason': u'The request transaction is larger than 10MB', u'error': u'transaction_too_large'}

make elixir tests=test/elixir/test/bulk_docs_test.exs
make exunit tests=src/couchdb/test/elixir/test/bulk_docs_test.exs

BulkDocsTest
  * test bulk docs raises error for invlaid `docs` parameter (75.5ms)
  * test bulk docs supplies `id` if not provided in doc (38.9ms)
  * test bulk docs can detect conflicts (64.4ms)
  * test bulk docs raises error for invlaid `new_edits` parameter (13.3ms)
  * test bulk docs raises error for missing `docs` parameter (13.0ms)
  * test bulk docs raises transaction_too_large error for transaction larger than 10MB (1384.9ms)
  * test bulk docs raises error for `all_or_nothing` option (13.6ms)
  * test bulk docs raises conflict error for combined update & delete (40.6ms)
  * test bulk docs emits conflict error for duplicate doc `_id`s (23.2ms)
  * test bulk docs can create, update, & delete many docs per request (78.3ms)


Finished in 2.0 seconds
10 tests, 0 failures

Randomized with seed 913112

Related Issues or Pull Requests

Checklist

Code is written and works correctly
Changes are covered by tests
Any new configurable parameters are documented in rel/overlay/etc/default.ini
A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation

bessbd

The change looks good to me, +1! (non-binding)
I wonder if it's easy to add a test case for this.

bessbd · 2020-10-13T10:02:39Z

src/chttpd/src/chttpd.erl

@@ -362,6 +362,8 @@ catch_error(HttpReq, error, decryption_failed) ->
 send_error(HttpReq, decryption_failed);
 catch_error(HttpReq, error, not_ciphertext) ->
 send_error(HttpReq, not_ciphertext);
+catch_error(HttpReq, error, {erlfdb_error,2101}) ->
+ send_error(HttpReq, {request_transcation_too_large, 2101});


Nit:

Suggested change

send_error(HttpReq, {request_transcation_too_large, 2101});

send_error(HttpReq, {request_transaction_too_large, 2101});

We could probably just use the transaction_too_large atom instead of the tuple {request_transaction_too_large, 2101} since the tag explains the error pretty well.

And we'd want to stay somewhat consistent with https://apple.github.io/foundationdb/api-error-codes.html

transaction_too_large | 2101 | Transaction exceeds byte limit

nickva · 2020-10-14T02:58:25Z

src/chttpd/src/chttpd.erl

@@ -362,6 +362,8 @@ catch_error(HttpReq, error, decryption_failed) ->
 send_error(HttpReq, decryption_failed);
 catch_error(HttpReq, error, not_ciphertext) ->
 send_error(HttpReq, not_ciphertext);
+catch_error(HttpReq, error, {erlfdb_error,2101}) ->


let's leave a space after , and 2101

nickva · 2020-10-14T03:08:03Z

src/chttpd/src/chttpd.erl

+error_info({request_transcation_too_large, Code}) ->
+ CodeBin = integer_to_binary(Code),
+ {413, <<"request_transcation_too_large">>,
+ <<"The request transcation is too large. (", CodeBin/binary, ")" >>};


Users might confuse 2101 with the transaction size limit. Since we know the limit as 10MB we can just include in the message perhaps something like {413, <<"transaction_too_large">>, <<"The request transaction is larger than 10MB">>}

nickva

Great improvement!

Made a few suggestions about simplifying the error names.

Good idea by @bessbd to add tests. Maybe in fabric2_update_docs_tests or if you can find a better place. Just make sure to generate random data so compression doesn't undo your large data generation.

Then it should be good to go.

As an extra aside to this PR, and mainly for discussion, it is interesting that max doc size doesn't take effect here. We should double check why does max_document_size limit of 8MB from default.ini not work? Is it because the check there is approximate (json "size") or, there is a bug somewhere with _bulk_docs. What if we lowered the max doc size limit to 4MB would we start getting a max doc size error then...?

jiangphcn · 2020-10-14T05:54:17Z

thanks @nickva and @bessbd. Good suggestions. Looks that I am in the right way, and I will refine message and add test.

davisp

+1

jiangphcn · 2020-10-16T04:17:12Z

thanks @davisp's review.

@nickva @bessbd I used d4557f2 to address your comments. Could you please take another look?

bessbd · 2020-10-16T07:09:19Z

LGTM. Thank you for the test case, @jiangphcn

bessbd · 2020-10-16T07:10:29Z

src/chttpd/src/chttpd.erl

@@ -362,6 +362,8 @@ catch_error(HttpReq, error, decryption_failed) ->
 send_error(HttpReq, decryption_failed);
 catch_error(HttpReq, error, not_ciphertext) ->
 send_error(HttpReq, not_ciphertext);
+catch_error(HttpReq, error, {erlfdb_error, 2101}) ->
+ send_error(HttpReq, transcation_too_large);


Sorry, just realized there's a typo here (and on line 1014):

Suggested change

send_error(HttpReq, transcation_too_large);

send_error(HttpReq, transaction_too_large);

Good catch, and fixed in 4f7caf7

nickva · 2020-10-16T18:05:47Z

test/elixir/test/bulk_docs_test.exs

+ test "bulk docs raises transaction_too_large error for transaction larger than 10MB", ctx do
+ docs = [%{_id: "0", a: random_string(16_000_000)}]
+ resp = Couch.post("/#{ctx[:db_name]}/_bulk_docs", body: %{docs: docs})
+ assert resp.status_code == 413


Let's match on the error message ("transaction_too_large") since we're testing for it.

Wondering if with 16MB we would hit transaction_too_large or the document size limit of 8MB. If we don't hit 8MB we may have another bug to fix about why that doesn't work.

There are different settings on CouchDB and Cloudant for Transaction Engine for max_document_size.

max_document_size = 8000000 ; bytes (CouchDB) max_document_size = 67108864 ; 64 MB (Cloudant for Transaction Engine)

So by default, we will get document_too_large in CouchDB while we will get transaction_too_large if we pass 16MB document.

I added codes to adjust max_document_size in this test to support 64MB documents to get transaction_too_large in any cases.

@jiangphcn

Good find!

TxE is technically Cloudant's problem and shouldn't have any relevance for CouchDB. Though looking at the code there, is that the /dev/run instance may have 65Mb however the instance used in production has 1Mb.

Also maybe in a separate commit, we could probably switch to using the code default (in Erlang) to be 8Mb instead of keeping in the default.ini. Then default.ini can be a commented out line with the default value in it like we usually do it.

nickva · 2020-10-19T18:39:11Z

test/elixir/test/bulk_docs_test.exs

+ @tag :with_db
+ test "bulk docs raises transaction_too_large error for transaction larger than 10MB", ctx do
+ docs = [%{_id: "0", a: random_string(16_000_000)}]
+ set_config_raw("couchdb", "max_document_size", "67108864")


Will this automatically reset if the test ends (either ok or fails)? We wouldn't want to alter the max doc size for other tests here.

No, the setting on [couchdb][max_document_size] will not be reverted after one test or test suite ends. I added codes to revert them. I believe that currently we are fine with this test.

nickva

+1 After double-checking that the config changes are reverted after the test finishes

nickva · 2020-10-20T14:13:39Z

@jiangphcn One more thing is we'd have to merge this to main since we use main as the 4.x branch

jiangphcn · 2020-10-20T14:19:11Z

Good reminder, @nickva. I will do soon.

wohali · 2020-10-20T15:11:32Z

Yeah, we should be careful, prototype/fdb-layer is slated to be removed this week.

jiangphcn requested a review from nickva October 13, 2020 04:36

jiangphcn force-pushed the transaction_too_large branch from d0404f0 to ad48e5a Compare October 13, 2020 04:39

bessbd reviewed Oct 13, 2020

View reviewed changes

nickva reviewed Oct 14, 2020

View reviewed changes

davisp approved these changes Oct 15, 2020

View reviewed changes

jiangphcn force-pushed the transaction_too_large branch from f33459d to cccdcdf Compare October 16, 2020 04:12

jiangphcn force-pushed the transaction_too_large branch from cccdcdf to d4557f2 Compare October 16, 2020 06:48

bessbd reviewed Oct 16, 2020

View reviewed changes

jiangphcn force-pushed the transaction_too_large branch from d4557f2 to 4f7caf7 Compare October 16, 2020 07:23

nickva requested changes Oct 16, 2020

View reviewed changes

jiangphcn force-pushed the transaction_too_large branch 2 times, most recently from a0ba69a to 7e97a04 Compare October 19, 2020 03:02

nickva reviewed Oct 19, 2020

View reviewed changes

nickva approved these changes Oct 19, 2020

View reviewed changes

jiangphcn force-pushed the transaction_too_large branch 7 times, most recently from 9f3fb54 to e275669 Compare October 20, 2020 04:51

convert erlfdb_error 2101 to transaction_too_large

640c173

jiangphcn force-pushed the transaction_too_large branch from e275669 to 640c173 Compare October 20, 2020 05:03

jiangphcn merged commit 7bdb432 into prototype/fdb-layer Oct 20, 2020

jiangphcn deleted the transaction_too_large branch October 20, 2020 12:49

jiangphcn mentioned this pull request Oct 21, 2020

convert erlfdb_error 2101 to transaction_too_large for main #3222

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

convert erlfdb_error 2101 to transaction_too_large #3215

convert erlfdb_error 2101 to transaction_too_large #3215

jiangphcn commented Oct 13, 2020 •

edited

Loading

bessbd left a comment

bessbd Oct 13, 2020

nickva Oct 14, 2020

nickva Oct 14, 2020

nickva Oct 14, 2020

nickva left a comment

jiangphcn commented Oct 14, 2020

davisp left a comment

jiangphcn commented Oct 16, 2020 •

edited

Loading

bessbd commented Oct 16, 2020

bessbd Oct 16, 2020 •

edited

Loading

jiangphcn Oct 16, 2020

nickva Oct 16, 2020

jiangphcn Oct 19, 2020

nickva Oct 19, 2020

nickva Oct 19, 2020

jiangphcn Oct 20, 2020

nickva left a comment

nickva commented Oct 20, 2020

jiangphcn commented Oct 20, 2020

wohali commented Oct 20, 2020

	send_error(HttpReq, {request_transcation_too_large, 2101});
	send_error(HttpReq, {request_transaction_too_large, 2101});

	send_error(HttpReq, transcation_too_large);
	send_error(HttpReq, transaction_too_large);

convert erlfdb_error 2101 to transaction_too_large #3215

convert erlfdb_error 2101 to transaction_too_large #3215

Conversation

jiangphcn commented Oct 13, 2020 • edited Loading

Overview

Testing recommendations

Related Issues or Pull Requests

Checklist

bessbd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickva left a comment

Choose a reason for hiding this comment

jiangphcn commented Oct 14, 2020

davisp left a comment

Choose a reason for hiding this comment

jiangphcn commented Oct 16, 2020 • edited Loading

bessbd commented Oct 16, 2020

bessbd Oct 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickva left a comment

Choose a reason for hiding this comment

nickva commented Oct 20, 2020

jiangphcn commented Oct 20, 2020

wohali commented Oct 20, 2020

jiangphcn commented Oct 13, 2020 •

edited

Loading

jiangphcn commented Oct 16, 2020 •

edited

Loading

bessbd Oct 16, 2020 •

edited

Loading