You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by gilv <gi...@git.apache.org> on 2015/10/14 13:05:54 UTC
[GitHub] couchdb-chttpd pull request: COUCHDB-769: Store attachments in the...
GitHub user gilv opened a pull request:
https://github.com/apache/couchdb-chttpd/pull/82
COUCHDB-769: Store attachments in the external storage.
Initial implementation that allows CouchDB to store attachments outside of the database file.
This implementation supports OpenStack Swift and SoftLayer Object store
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/gilv/couchdb-chttpd store_attachments_external
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-chttpd/pull/82.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 #82
----
commit 3f77f53eab5c28f5e291a36b1a39b93719c992e3
Author: Gil Vernik <gi...@il.ibm.com>
Date: 2015-10-14T10:45:53Z
COUCHDB-769: Store attachments in external storage. This is initial implementation, supports OpenStack Swift and SoftLayer Object store
----
---
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: COUCHDB-769: Store attachments in the...
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/82#discussion_r41985231
--- Diff: src/chttpd_db.erl ---
@@ -1127,12 +1155,20 @@ db_attachment_req(#httpd{method='GET',mochi_req=MochiReq}=Req, Db, DocId, FileNa
% header we'll fall back to a chunked response.
undefined
end,
- AttFun = case ReqAcceptsAttEnc of
+ AttFunTmp = case ReqAcceptsAttEnc of
false ->
fun couch_att:foldl_decode/3;
true ->
fun couch_att:foldl/3
end,
+ case AttExternal of
+ "external" ->
+ AttFun = couch_att:fetch(data,Att),
+ couch_log:debug("chtttpd_db: Got new attachment ",[]);
+ _ ->
+ couch_log:debug("chttpd_db: not external att",[]),
+ AttFun = AttFunTmp
--- End diff --
So, AttFun could be actually an attachment data or some fold function? Something went wrong if you have to do this.
---
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: COUCHDB-769: Store attachments in the...
Posted by gilv <gi...@git.apache.org>.
Github user gilv commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-168609316
@kxepal @rnewson
I provided another patch, implementing most of the comments.
Here is what is not implemented in this patch:
1. Remark on the AttFun. Not yet implemented. Original remark: “AttFun could be actually an attachment data or some fold function? Something went wrong if you have to do this.”
2. usage of couch_util:to_hex
3. still no unitests
---
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: COUCHDB-769: Store attachments in the...
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/82#discussion_r48728274
--- Diff: src/chttpd_db.erl ---
@@ -722,7 +731,14 @@ db_doc_req(#httpd{method='POST', user_ctx=Ctx}=Req, Db, DocId) ->
NewDoc = Doc#doc{
atts = UpdatedAtts ++ OldAtts2
},
- case fabric:update_doc(Db, NewDoc, Options) of
+ NewNewDoc = case fabric_att_handler:external_store() of
+ true ->
+ fabric_att_handler:att_store(Db, NewDoc); %store_single_document
+ false ->
--- End diff --
indentation
---
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: COUCHDB-769: Store attachments in the...
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/82#discussion_r48728194
--- Diff: src/chttpd_db.erl ---
@@ -407,7 +407,16 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
true -> [all_or_nothing|Options];
_ -> Options
end,
- case fabric:update_docs(Db, Docs, Options2) of
+ case fabric_att_handler:external_store() of
+ true ->
+ couch_log:debug("store attachment externaly", []),
+ NewDocs = fabric_att_handler:att_store(Db,
+ fabric:docs(Docs));
+ false ->
+ couch_log:debug("externalize attachment: disabled",[]),
+ NewDocs = Docs
+ end,
--- End diff --
It would be cleaner if the result of the case statement was assigned to `NewDocs`. i.e, `NewDocs = case...`
---
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: COUCHDB-769: Store attachments in the...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-169617505
@gilv It's not a bad request since it's not a user fault that Swift is down. HTTP 503 or HTTP 502 looks good here. But yes, the only thing you can do is to fail and throw error back.
Btw, what's the timeout you use for fetching remote attachments? Do you stream response from Swift or buffer it in memory before return back to user?
---
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: COUCHDB-769: Store attachments in the...
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/82#discussion_r41981123
--- Diff: src/chttpd_db.erl ---
@@ -407,7 +407,15 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
true -> [all_or_nothing|Options];
_ -> Options
end,
- case fabric:update_docs(Db, Docs, Options2) of
+ couch_log:debug("chttpd_db: update_docs wellcome. Going to store attachment in external store",[]),
--- End diff --
welcome! (:
---
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: COUCHDB-769: Store attachments in the...
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/82#discussion_r48728241
--- Diff: src/chttpd_db.erl ---
@@ -407,7 +407,16 @@ db_req(#httpd{method='POST',path_parts=[_,<<"_bulk_docs">>], user_ctx=Ctx}=Req,
true -> [all_or_nothing|Options];
_ -> Options
end,
- case fabric:update_docs(Db, Docs, Options2) of
+ case fabric_att_handler:external_store() of
+ true ->
+ couch_log:debug("store attachment externaly", []),
+ NewDocs = fabric_att_handler:att_store(Db,
+ fabric:docs(Docs));
+ false ->
+ couch_log:debug("externalize attachment: disabled",[]),
+ NewDocs = Docs
+ end,
--- End diff --
hm, and really, it should be `fabric:update_docs` itself that makes this decision, the http presentation layer should not know.
---
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: COUCHDB-769: Store attachments in the...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-168672529
summary: there's too much logic in the http layer, fabric should be the place where the decisions are made.
---
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: COUCHDB-769: Store attachments in the...
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/82#discussion_r48728304
--- Diff: src/chttpd_db.erl ---
@@ -722,7 +731,14 @@ db_doc_req(#httpd{method='POST', user_ctx=Ctx}=Req, Db, DocId) ->
NewDoc = Doc#doc{
atts = UpdatedAtts ++ OldAtts2
},
- case fabric:update_doc(Db, NewDoc, Options) of
+ NewNewDoc = case fabric_att_handler:external_store() of
+ true ->
+ fabric_att_handler:att_store(Db, NewDoc); %store_single_document
+ false ->
+ couch_log:debug("Store inline attachmets in Swift disabled",[]),
--- End diff --
this mentions Swift when it shouldn't (it might be stored there)
---
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: COUCHDB-769: Store attachments in the...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-148030303
Is it possible to decouple logic of working with external attachments from the logic that works with internal ones? Mixing them makes quite hard to follow the track.
---
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: COUCHDB-769: Store attachments in the...
Posted by gilv <gi...@git.apache.org>.
Github user gilv commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-176590336
@kxepal Can you please review the recent code?
---
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: COUCHDB-769: Store attachments in the...
Posted by gilv <gi...@git.apache.org>.
Github user gilv commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-167132132
@kxepal : you asked to "Is it possible to decouple logic of working with external attachments from the logic that works with internal ones? Mixing them makes quite hard to follow the track."
Can you please explain me what would you like to see exactly? Perhaps i didn't understand it.
Currently fabric_attachment_handler is the one that responsible to store attachments externally. This module does't really care if it's internal or external, they both go to the same object store.
---
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: COUCHDB-769: Store attachments in the...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-148027542
Would be nice to see some tests for this to prove that this all works.
---
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: COUCHDB-769: Store attachments in the...
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/82#discussion_r41984976
--- Diff: src/chttpd_db.erl ---
@@ -733,7 +741,17 @@ db_doc_req(#httpd{method='POST', user_ctx=Ctx}=Req, Db, DocId) ->
NewDoc = Doc#doc{
atts = UpdatedAtts ++ OldAtts2
},
- case fabric:update_doc(Db, NewDoc, Options) of
+ couch_log:debug("chttpd_db: couchNew doc formed including new att: formed",[]),
+ lists:foreach(fun(X) -> couch_log:debug("~p~n ~p~n",[couch_att:fetch(data, X), couch_att:fetch(name, X)]) end, NewDoc#doc.atts),
+ couch_log:debug("chttpd_db: going to call attachment handler for base64 attachments",[]),
+ case fabric_attachments_handler:externalize_att(Db) of
+ "true" ->
+ NewNewDoc = fabric_attachments_handler:inline_att_store(Db, NewDoc); %store_single_document
+ "false" ->
+ couch_log:debug("Store inline attachmets in Swift disabled",[]),
+ NewNewDoc = NewDoc
--- End diff --
Better not assign variable in case, but assign case result to variable.
---
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: COUCHDB-769: Store attachments in the...
Posted by gilv <gi...@git.apache.org>.
Github user gilv commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-169601987
@kxepal I need your advice on the following:
Consider the following use case:
1. Attachment is stored externally
2. User download attachment
3. CouchDB fails to download attachment from Swift ( for example network failure )
The relevant code in "chttpd_db" is:
ExtStoreID = couch_att:fetch(att_extstore_id,TmpAtt),
case ExtStoreID of
undefined ->
couch_log:debug("chttpd_db: Att is not stored in the external store ~p~n",[ExtStoreID]),
Att = TmpAtt;
_ ->
Att = fabric_att_handler:att_get(Db, TmpAtt),
couch_log:debug("chtttpd_db: Got attachment from the external store ~p~n",[ExtStoreID])
end,
My question is: what is the best way to handle this?
Should i do something like this:
case fabric_att_handler:att_get(Db, TmpAtt),
{ok, Att} ->
couch_log:debug("chtttpd_db: Got attachment from the external store ~p~n",[ExtStoreID])
{error,_} ->
throw({
bad_request,
"Please try again later"
})
.....
---
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: COUCHDB-769: Store attachments in the...
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/82#discussion_r41985112
--- Diff: src/chttpd_db.erl ---
@@ -733,7 +741,17 @@ db_doc_req(#httpd{method='POST', user_ctx=Ctx}=Req, Db, DocId) ->
NewDoc = Doc#doc{
atts = UpdatedAtts ++ OldAtts2
},
- case fabric:update_doc(Db, NewDoc, Options) of
+ couch_log:debug("chttpd_db: couchNew doc formed including new att: formed",[]),
+ lists:foreach(fun(X) -> couch_log:debug("~p~n ~p~n",[couch_att:fetch(data, X), couch_att:fetch(name, X)]) end, NewDoc#doc.atts),
--- End diff --
Please, don't do this: if document has a lot of attachments, this loop will spin without producing any useful work, but only slow down all around.
---
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: COUCHDB-769: Store attachments in the...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-148022732
Please fix:
- Style: try to fit 80 chars line length, put space after comma, no tabs, correctly aligned clauses etc.
- Reduce amount of debug logs: debug logs should help you understand what going on in your code and in which state it is in places where problems may happens, not produce noise about every logical step.
---
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: COUCHDB-769: Store attachments in the...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-167132910
@gilv I mean to get rid all `case AttExternal of "external" -> ...; _ -> ... end` and instead have some function(s) with two clauses: one to process attachments stored inside db file and other - in some external storage. This makes logic of processing internal and external attachments separated, clean and easy to track. This also helps to avoid situations like [this one](https://github.com/apache/couchdb-chttpd/pull/82/files#r41985231) when depending on attachment type.
---
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: COUCHDB-769: Store attachments in the...
Posted by rnewson <gi...@git.apache.org>.
Github user rnewson commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-168670685
Given the nature of the patch, this cannot merge without tests, assuming all style and other issues are resolved.
---
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: COUCHDB-769: Store attachments in the...
Posted by gilv <gi...@git.apache.org>.
Github user gilv commented on the pull request:
https://github.com/apache/couchdb-chttpd/pull/82#issuecomment-173118935
@kxepal @rnewson
Few notes related recent patch
1. Please ignore all debug / info print statements. At some point i will remove them at once.
2. I addressed most of the comments.
3. Remark on the AttFun. Not yet implemented. Original remark: “AttFun could be actually an attachment data or some fold function? Something went wrong if you have to do this.”
4. Still no unitests / functional tests.
5. Attachment retrieval from the object store - keep all data in memory. I need to figure out how to use streaming there.
---
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.
---