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/01/19 21:25:46 UTC
[GitHub] couchdb-couch pull request: Make sure we load all applications wit...
GitHub user iilyak opened a pull request:
https://github.com/apache/couchdb-couch/pull/32
Make sure we load all applications with stats defs
couch_stats app relies on application:loaded_applications.
While in the test suite we have to load the apps manually
prior to start couch_stats app
COUCHDB-2552
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/iilyak/couchdb-couch 2552-unknown-metric-mrview-map_docs
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-couch/pull/32.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 #32
----
commit b528ef92abfedd8e6025becc5f8b60cb02168e59
Author: ILYA Khlopotov <ii...@ca.ibm.com>
Date: 2015-01-19T20:19:29Z
Make sure we load all applications with stats defs
couch_stats app relies on application:loaded_applications.
While in the test suite we have to load the apps manually
prior to start couch_stats app
COUCHDB-2552
----
---
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-couch issue #32: Make sure we load all applications with stats defs
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the issue:
https://github.com/apache/couchdb-couch/pull/32
stale PR
---
[GitHub] couchdb-couch pull request: Make sure we load all applications wit...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch/pull/32#issuecomment-70786078
> BTW I've tried to use start_couch with [couch_mrview] extra app. It doesn't work in this case due to complex dependencies between couch_stats, couch_logger, couch_index and couch_mrview.
Works for me. Example change:
```diff
diff --git a/src/couch_mrview_updater.erl b/src/couch_mrview_updater.erl
index db5b5e9..81b0085 100644
--- a/src/couch_mrview_updater.erl
+++ b/src/couch_mrview_updater.erl
@@ -187,7 +187,7 @@ map_docs(Parent, State0) ->
({Id, Seq, Rev, deleted}, {SeqAcc, Results}) ->
{erlang:max(Seq, SeqAcc), [{Id, Seq, Rev, []} | Results]};
({Id, Seq, Rev, Doc}, {SeqAcc, Results}) ->
- couch_stats:increment_counter([couchdb, mrview, map_docs],
+ couch_stats:increment_counter([couchdb, mrview, map_doc],
1),
{ok, Res} = couch_query_servers:map_doc_raw(QServer, Doc),
{erlang:max(Seq, SeqAcc), [{Id, Seq, Rev, Res} | Results]}
diff --git a/test/couch_mrview_red_views_tests.erl b/test/couch_mrview_red_views_tests.erl
index 8c2ff47..2a07bcc 100644
--- a/test/couch_mrview_red_views_tests.erl
+++ b/test/couch_mrview_red_views_tests.erl
@@ -18,6 +18,9 @@
-define(TIMEOUT, 1000).
+start()->
+ test_util:start_couch([couch_mrview]).
+
setup() ->
{ok, Db} = couch_mrview_test_util:init_db(?tempdb(), red),
Db.
@@ -33,7 +36,7 @@ reduce_views_test_() ->
"Reduce views",
{
setup,
- fun test_util:start_couch/0, fun test_util:stop_couch/1,
+ fun start/0, fun test_util:stop_couch/1,
{
foreach,
fun setup/0, fun teardown/1,
```
---
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-couch pull request: Make sure we load all applications wit...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/32#issuecomment-70725407
@kxepal BTW I've tried to use start_couch with [couch_mrview] extra app. It doesn't work in this case due to complex dependencies between `couch_stats`, `couch_logger`, `couch_index` and `couch_mrview`.
---
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-couch pull request #32: Make sure we load all applications with stat...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak closed the pull request at:
https://github.com/apache/couchdb-couch/pull/32
---
[GitHub] couchdb-couch pull request: Make sure we load all applications wit...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-couch/pull/32#issuecomment-70641516
Yes, this fixes the issue. However, not sure about the fix correctness.
`start_couch` is basically starts minimal CouchDB instance with the specified config files (if any). Additionally, it may be instructed to start some more apps which implements some features, like: couch_mrview, couch_replicator etc.
This fix tries to break this and actually start minimal CouchDB instance + all the apps with the stats definition. This isn't minimalistic instance anymore, but not yet complete one.
So the question is: do we ever need in such startup definition (minimal core+extra apps)? May be we need to start CouchDB with all the apps all the time? Or even be more flexible: provide `start_minimal_couch([IniFiles, [ExtraApps]])` and `start_full_couch([IniFiles])` - what does it gives us? /cc @chewbranca
The more finest fix for the 2552 is to touch couch_mrview tests setup function to call `start_couch` with `[couch_mrview]` extra app like couch_replicator does.
---
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-couch pull request: Make sure we load all applications wit...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/32#issuecomment-70678446
@kxepal [application:load/1 <> application:start/1](https://github.com/iilyak/couchdb-couch/blob/2552-unknown-metric-mrview-map_docs/src/test_util.erl#L144)
---
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-couch pull request: Make sure we load all applications wit...
Posted by iilyak <gi...@git.apache.org>.
Github user iilyak commented on the pull request:
https://github.com/apache/couchdb-couch/pull/32#issuecomment-70682346
While working on test suite I had a need for start_couch(ExtraApps) and stop_couch(ExtraApps). I figure out that this API actually not side-effect free. Since when we do start_couch(Apps) we start dependencies. I tend to change it to start_couch/1 -> [started_apps]. So we can pass list of apps to teardown function. I think it is a sensible change. However this will affect lot's of repositories. There is an alternative to it which is to start a named process which would act as a term storage for the current test. Store arbitrary terms in the state of that process including info about apps that needs to be stopped on teardown. This would reduce number of arguments we would need to pass to tests themselves. We also can use unnamed process and pass Pid of it to every test as a Context and have test_util:get(Context, Key) and [Arg1, Arg2] = test_util:get(Context, [key1, key2]) to extract args from context. Not sure about the best approach to this. The change is out of the scope of thi
s ticket anyway.
---
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.
---