You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@couchdb.apache.org by iilyak <gi...@git.apache.org> on 2015/04/29 21:20:37 UTC
[GitHub] couchdb-chttpd pull request: Handle Etag generation in one place
GitHub user iilyak opened a pull request:
https://github.com/apache/couchdb-chttpd/pull/34
Handle Etag generation in one place
COUCHDB-2650
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/iilyak/couchdb-chttpd 2650-etag-fixes
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-chttpd/pull/34.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #34
----
commit 98a372680c1251fce47c5af8da931be409d9756a
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-04-29T18:39:55Z
Handle Etag generation in one place
COUCHDB-2650
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: Handle Etag generation in one place
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/34#issuecomment-97563690
Should I call `chttpd`'s version of etag related functions from `couch` application in cases [like](https://github.com/apache/couchdb-couch/blob/952a41c7a10fc4dcb0324d09c5be7e5d1d548803/src/couch_httpd_db.erl#L121). Alternatively I can update `couch_chttpd` to dispatch the call to `chttpd`. Which of the above you would prefer?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: Handle Etag generation in one place
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/34#issuecomment-104318793
Ah, found it. Reviewing the others as well.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd issue #34: Handle Etag generation in one place
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the issue:
https://github.com/apache/couchdb-chttpd/pull/34
stale PR
---
[GitHub] couchdb-chttpd pull request: Handle Etag generation in one place
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/34#discussion_r29371280
--- Diff: src/chttpd.erl ---
@@ -535,7 +535,15 @@ json_body_obj(Httpd) ->
doc_etag(#doc{revs={Start, [DiskRev|_]}}) ->
- "\"" ++ ?b2l(couch_doc:rev_to_str({Start, DiskRev})) ++ "\"".
+ rev_etag({Start, DiskRev}).
+
+rev_etag({Start, DiskRev}) ->
+ Rev = couch_doc:rev_to_str({Start, DiskRev}),
+ <<"\"", Rev/binary, "\"">>.
+
+make_etag() ->
--- End diff --
a random uuid is essentially useless. The only reason cloudant does that (for views) is to preserve the shape of the code relative to couch. Better to omit the etag header entirely if it isn't going to be stable.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request #34: Handle Etag generation in one place
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak closed the pull request at:
https://github.com/apache/couchdb-chttpd/pull/34
---
[GitHub] couchdb-chttpd pull request: Handle Etag generation in one place
Posted by davisp <gi...@git.apache.org>.
Github user davisp commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/34#issuecomment-104318654
This looks mostly good to me. I'm confused by the changes to add header manipulation functions in 9ab5b83 which don't appear to be used anywhere. Is there a corresponding branch in couch_mrview that's not linked here?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: Handle Etag generation in one place
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/34#discussion_r29424678
--- Diff: src/chttpd.erl ---
@@ -535,7 +535,15 @@ json_body_obj(Httpd) ->
doc_etag(#doc{revs={Start, [DiskRev|_]}}) ->
- "\"" ++ ?b2l(couch_doc:rev_to_str({Start, DiskRev})) ++ "\"".
+ rev_etag({Start, DiskRev}).
+
+rev_etag({Start, DiskRev}) ->
+ Rev = couch_doc:rev_to_str({Start, DiskRev}),
+ <<"\"", Rev/binary, "\"">>.
+
+make_etag() ->
--- End diff --
@rnewson: I don't think changing how `views` handle etag generation is in the scope of the bug. I could work on it in the context of another bug if you think it needs to be done before 2.0 release. I would approach this by putting `#mrst.sig` into meta. Then when we get [here](https://github.com/apache/couchdb-couch-mrview/blob/master/src/couch_mrview_http.erl#L331), we can use `#mrst.sig` to calculate etag instead of relying on `#vacc.etag` (which is uuid at this point).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: Handle Etag generation in one place
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/34#issuecomment-97551446
Good idea! May be also extract attachment etag generation for consistency?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: Handle Etag generation in one place
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/34#issuecomment-97567883
I think not if only you're going to slowly replace couch_httpd by chttpd. Otherwise, it will make more complicated references between subprojects (they are already quite complicated).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] couchdb-chttpd pull request: Handle Etag generation in one place
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on a diff in the pull request:
https://github.com/apache/couchdb-chttpd/pull/34#discussion_r29372386
--- Diff: src/chttpd.erl ---
@@ -535,7 +535,15 @@ json_body_obj(Httpd) ->
doc_etag(#doc{revs={Start, [DiskRev|_]}}) ->
- "\"" ++ ?b2l(couch_doc:rev_to_str({Start, DiskRev})) ++ "\"".
+ rev_etag({Start, DiskRev}).
+
+rev_etag({Start, DiskRev}) ->
+ Rev = couch_doc:rev_to_str({Start, DiskRev}),
+ <<"\"", Rev/binary, "\"">>.
+
+make_etag() ->
--- End diff --
Why it was used in CouchDB then? Not because to overcome some cache issues?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---