You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by GitBox <gi...@apache.org> on 2021/01/26 13:49:35 UTC
[GitHub] [couchdb] bessbd opened a new pull request #3347: Set a finite default for max_attachment_size
bessbd opened a new pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347
## Overview
The current default for `max_attachment_size` is `infinity`. This PR changes that to 1 gibibyte.
## Testing recommendations
No testing, this is just a default change.
## Related Issues or Pull Requests
N/A
## 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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] rnewson commented on a change in pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
rnewson commented on a change in pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#discussion_r566349531
##########
File path: src/couch/src/couch_att.erl
##########
@@ -178,6 +178,9 @@
-type att() :: #att{} | attachment() | disk_att().
+% 1024 * 1024 * 1024
+-define(GB, 1073741824).
Review comment:
better to say `-define(GB, (1024*1024*1024)).` I'm sure the compiler will do the right hing.
##########
File path: rel/overlay/etc/default.ini
##########
@@ -47,7 +47,7 @@ changes_doc_ids_optimization_threshold = 100
max_document_size = 8000000 ; bytes
;
; Maximum attachment size.
-; max_attachment_size = infinity
+; max_attachment_size = 1073741824 ; 1 gibibyte
Review comment:
I'd go even lower tbh but anything is an improvement over unlimited.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#issuecomment-768816084
Thank you for the review, @iilyak !
I've just opened vote thread: https://lists.apache.org/thread.html/r815b25fe34996ab3c54e2fb9759de2026ddccffc8bb59966b1168063%40%3Cdev.couchdb.apache.org%3E
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on a change in pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#discussion_r565098575
##########
File path: src/couch/src/couch_att.erl
##########
@@ -732,13 +735,19 @@ upgrade_encoding(true) -> gzip;
upgrade_encoding(false) -> identity;
upgrade_encoding(Encoding) -> Encoding.
-
max_attachment_size() ->
- case config:get("couchdb", "max_attachment_size", "infinity") of
+ max_attachment_size(config:get("couchdb", "max_attachment_size", ?GB)).
+
+max_attachment_size(MaxAttSizeConfig) ->
+ case MaxAttSizeConfig of
"infinity" ->
infinity;
+ MaxAttSize when is_list(MaxAttSize) ->
+ list_to_integer(MaxAttSize);
+ MaxAttSize when is_integer(MaxAttSize) ->
Review comment:
Considering https://github.com/apache/couchdb/pull/3347#discussion_r564547120 , I'd hope going for a type check has better performance than a conversion. What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
iilyak commented on pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#issuecomment-767574082
This PR might require a vote on ML.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd merged pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
bessbd merged pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on a change in pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#discussion_r567995818
##########
File path: rel/overlay/etc/default.ini
##########
@@ -47,7 +47,7 @@ changes_doc_ids_optimization_threshold = 100
max_document_size = 8000000 ; bytes
;
; Maximum attachment size.
-; max_attachment_size = infinity
+; max_attachment_size = 1073741824 ; 1 gibibyte
Review comment:
I support decreasing this over time. I wouldn't want to change this number right now, because I believe that would change the scope of the ongoing vote on the ML.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#issuecomment-771087608
Thank you for the review, @iilyak and @rnewson !
I'm about to merge this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on a change in pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#discussion_r564566372
##########
File path: rel/overlay/etc/default.ini
##########
@@ -47,7 +47,7 @@ changes_doc_ids_optimization_threshold = 100
max_document_size = 8000000 ; bytes
;
; Maximum attachment size.
-; max_attachment_size = infinity
+; max_attachment_size = 1073741824 : 1 gibibyte
Review comment:
Done in fa8288bdd
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#discussion_r564759503
##########
File path: src/couch/src/couch_att.erl
##########
@@ -732,13 +735,19 @@ upgrade_encoding(true) -> gzip;
upgrade_encoding(false) -> identity;
upgrade_encoding(Encoding) -> Encoding.
-
max_attachment_size() ->
- case config:get("couchdb", "max_attachment_size", "infinity") of
+ max_attachment_size(config:get("couchdb", "max_attachment_size", ?GB)).
+
+max_attachment_size(MaxAttSizeConfig) ->
+ case MaxAttSizeConfig of
"infinity" ->
infinity;
+ MaxAttSize when is_list(MaxAttSize) ->
+ list_to_integer(MaxAttSize);
+ MaxAttSize when is_integer(MaxAttSize) ->
Review comment:
You wouldn't need this clause if you change the define to be a string.
```
-define(GB, "1073741824").
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#discussion_r564550696
##########
File path: rel/overlay/etc/default.ini
##########
@@ -47,7 +47,7 @@ changes_doc_ids_optimization_threshold = 100
max_document_size = 8000000 ; bytes
;
; Maximum attachment size.
-; max_attachment_size = infinity
+; max_attachment_size = 1073741824 : 1 gibibyte
Review comment:
Use `;` instead of `:` to introduce comment.
`; max_attachment_size = 1073741824 : 1 gibibyte`
............................................................... ^----- should be `;`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#discussion_r564547120
##########
File path: src/couch/src/couch_att.erl
##########
@@ -734,7 +734,7 @@ upgrade_encoding(Encoding) -> Encoding.
max_attachment_size() ->
- case config:get("couchdb", "max_attachment_size", "infinity") of
+ case config:get("couchdb", "max_attachment_size", 1024 * 1024 * 1024) of
Review comment:
Erlang doesn't do compile time optimizations. I think multiplications would be slow. Could you use a define instead.
```
% 1024 * 1024 * 1024
-define(GB, 1073741824).
....
max_attachment_size() ->
case config:get("couchdb", "max_attachment_size", ?GB) of
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] iilyak commented on a change in pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
iilyak commented on a change in pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#discussion_r564761679
##########
File path: src/couch/src/couch_att.erl
##########
@@ -732,13 +735,19 @@ upgrade_encoding(true) -> gzip;
upgrade_encoding(false) -> identity;
upgrade_encoding(Encoding) -> Encoding.
-
max_attachment_size() ->
- case config:get("couchdb", "max_attachment_size", "infinity") of
+ max_attachment_size(config:get("couchdb", "max_attachment_size", ?GB)).
+
+max_attachment_size(MaxAttSizeConfig) ->
+ case MaxAttSizeConfig of
"infinity" ->
infinity;
+ MaxAttSize when is_list(MaxAttSize) ->
+ list_to_integer(MaxAttSize);
+ MaxAttSize when is_integer(MaxAttSize) ->
+ MaxAttSize;
MaxAttSize ->
- list_to_integer(MaxAttSize)
+ erlang:error({invalid_max_attachment_size, MaxAttSize})
Review comment:
If we are throwing new type of error we might need to define it here https://github.com/apache/couchdb/blob/main/src/chttpd/src/chttpd.erl#L930 and maybe here https://github.com/apache/couchdb/blob/main/src/chttpd/src/chttpd.erl#L335
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on a change in pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#discussion_r567990860
##########
File path: src/couch/src/couch_att.erl
##########
@@ -178,6 +178,9 @@
-type att() :: #att{} | attachment() | disk_att().
+% 1024 * 1024 * 1024
+-define(GB, 1073741824).
Review comment:
Done in 83be3921b
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on a change in pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
bessbd commented on a change in pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#discussion_r565348997
##########
File path: src/couch/src/couch_att.erl
##########
@@ -732,13 +735,19 @@ upgrade_encoding(true) -> gzip;
upgrade_encoding(false) -> identity;
upgrade_encoding(Encoding) -> Encoding.
-
max_attachment_size() ->
- case config:get("couchdb", "max_attachment_size", "infinity") of
+ max_attachment_size(config:get("couchdb", "max_attachment_size", ?GB)).
+
+max_attachment_size(MaxAttSizeConfig) ->
+ case MaxAttSizeConfig of
"infinity" ->
infinity;
+ MaxAttSize when is_list(MaxAttSize) ->
+ list_to_integer(MaxAttSize);
+ MaxAttSize when is_integer(MaxAttSize) ->
+ MaxAttSize;
MaxAttSize ->
- list_to_integer(MaxAttSize)
+ erlang:error({invalid_max_attachment_size, MaxAttSize})
Review comment:
Good catch! What do you think of 921aa502e instead though?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [couchdb] bessbd commented on pull request #3347: Set a finite default for max_attachment_size
Posted by GitBox <gi...@apache.org>.
bessbd commented on pull request #3347:
URL: https://github.com/apache/couchdb/pull/3347#issuecomment-767603240
Thank you for the review, @iilyak . I've just noticed that this PR is not passing some tests. I'll push a commit soon to fix that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org