You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@couchdb.apache.org by jaydoane <gi...@git.apache.org> on 2015/11/02 03:14:32 UTC
[GitHub] couchdb-config pull request: Don't start lager during setup for ea...
GitHub user jaydoane opened a pull request:
https://github.com/apache/couchdb-config/pull/7
Don't start lager during setup for each config_test
BugzID: 55507
COUCHDB-2866
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloudant/couchdb-config 55507-config_tests-remove-lager
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/couchdb-config/pull/7.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 #7
----
commit 4d08e1c0c3a7bdd5bb71bab17c8b54b70ff934d4
Author: Jay Doane <ja...@gmail.com>
Date: 2015-11-02T02:11:17Z
Don't start lager during setup for each config_test
BugzID: 55507
COUCHDB-2866
----
---
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-config pull request: Don't start lager during setup for ea...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-154492458
@kxepal, if your head is feeling better, I was hoping you could share your thoughts on my latest approach?
---
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-config pull request: Don't start lager during setup for ea...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-175569950
I think +1. Shutdown notices a bit distracting, but that's another story, not related to this PR.
---
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-config pull request: Don't start lager during setup for ea...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-173510292
@jaydoane Trivial: because tests modifies config level and we now let it be changed in runtime.
https://github.com/apache/couchdb-config/blob/master/test/config_tests.erl#L260-L261
https://github.com/apache/couchdb-config/blob/master/test/config_tests.erl#L279-L280
Previously it was at error level without ability to be tweaked via config API, so any of these changes had no effect and you actually didn't see any log messages.
---
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-config pull request: Don't start lager during setup for ea...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-154503958
I agree that explicitly starting lager is not required for the tests to pass, but like you pointed out above, not doing so generates a bunch of supervisor related errors.
A little googling suggests we can perhaps ignore them, though:
http://stackoverflow.com/questions/19354937/strange-error-message-when-stopping-app-using-lager-and-poolboy
---
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-config pull request: Don't start lager during setup for ea...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-153906372
@jaydoane my head is going to blow today, will check tomorrow. Thanks for the update!
---
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-config pull request: Don't start lager during setup for ea...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-152925628
@kxepal, good point - I had not noticed those errors, and had just been happy that all the tests passed. I wonder if those errors are what's causing travis to fail?
In any case, looks like we need a better solution for this issue.
---
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-config pull request: Don't start lager during setup for ea...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-173819593
@kxepal, thank you for reminding me of that change.
I tracked down where the remaining logging is coming from in config_tests, and was able to remove a little bit more with the latest commit to this branch.
Of what's left, lines which look like:
```
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
```
are from https://github.com/apache/couchdb-config/blob/master/src/config.erl#L273, and appear to be normal, while logging like this:
```
[notice] config: [httpd] port set to 8080 for reason nil
```
is coming from https://github.com/apache/couchdb-config/blob/master/test/config_tests.erl#L320, and also seems harmless.
In the context of the tests being run which emit these log messages, where no .ini files are being used to initialize config, it doesn't appear practical to suppress the remaining log messages.
Are there any remaining objections to this PR?
---
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-config pull request: Don't start lager during setup for ea...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-152930791
@jaydoane Because travis config was a little bit outdated. I just updated it, please rebase for green light.
---
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-config pull request: Don't start lager during setup for ea...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-174659864
@kxepal, @iilyak thought I should reiterate that I can no longer reproduce the
`2015-11-02 08:31:53.274 [error] Undefined <0.885.0> Supervisor gr_counter_sup had child undefined started with gr_counter:start_link(gr_lager_default_tracer_counters) at <0.902.0> exit with reason killed in context child_terminated` errors we saw earlier, and it was their presence which seemed like your main objection to this PR.
---
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-config pull request: Don't start lager during setup for ea...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-153901692
@kxepal, I've cleaned up the code and added some comments. Please let me know if this is or is not an acceptable solution.
If not, another approach could be to factor this code:
https://github.com/apache/couchdb-couch-log/blob/master/src/couch_log.erl#L121-L141
into a utility module, and use it more widely to mock log and stats calls. We could presumably eliminate all dependencies but config, and likely speed up tests, etc.
---
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-config pull request: Don't start lager during setup for ea...
Posted by nickva <gi...@git.apache.org>.
Github user nickva commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-175380774
I checked this one against master using this patch
```
-{config, "config", "84197a6f1c5cb43447239df1fe57b4312b0c03c4"},
+%{config, "config", "84197a6f1c5cb43447239df1fe57b4312b0c03c4"},
+{config, {url, "https://github.com/cloudant/couchdb-config.git"},
+ {branch, "55507-config_tests-remove-lager"}
+},
```
Tested on Mac OS X, Erlang 18.2
Diff-ed output of `make eunit` with that without the patch and couldn't see any significant differences (test deltas, proc ids, some compilation order was different but nothing stood out).
So +1 from me, though @jaydoane might be better to squash the commits into one commit?
@kxepal what do you think?
---
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-config pull request: Don't start lager during setup for ea...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/couchdb-config/pull/7
---
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-config pull request: Don't start lager during setup for ea...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-152918941
This causes errors during tests run:
```
======================== EUnit ========================
module 'config_app'
module 'config_util'
module 'config_listener'
module 'config'
config: to_integer_test...ok
config: to_float_test...ok
module 'config_tests'
CouchDB config tests
Config get tests
config_tests:221: should_load_all_configs...[0.001 s] ok
2015-11-02 08:31:53.274 [error] Undefined <0.885.0> Supervisor gr_counter_sup had child undefined started with gr_counter:start_link(gr_lager_default_tracer_counters) at <0.902.0> exit with reason killed in context child_terminated
2015-11-02 08:31:53.274 [error] Undefined <0.886.0> Supervisor gr_param_sup had child undefined started with gr_param:start_link(gr_lager_default_tracer_params) at <0.901.0> exit with reason killed in context child_terminated
config_tests:224: should_locate_daemons_section...ok
config_tests:227: should_locate_mrview_handler...ok
config_tests:231: should_return_undefined_atom_on_missed_section...ok
config_tests:235: should_return_undefined_atom_on_missed_option...ok
config_tests:239: should_return_custom_default_value_on_missed_option...ok
config_tests:243: should_only_return_default_on_missed_option...ok
config_tests:247: should_fail_to_get_binary_value...ok
config_tests:254: should_return_any_supported_default (for type(undefined))...ok
config_tests:254: should_return_any_supported_default (for type("list"))...ok
config_tests:254: should_return_any_supported_default (for type(true))...ok
config_tests:254: should_return_any_supported_default (for type(false))...ok
config_tests:254: should_return_any_supported_default (for type(0.1))...ok
config_tests:254: should_return_any_supported_default (for type(1))...ok
[done in 0.597 s]
Config set tests
config_tests:258: should_update_option...[0.010 s] ok
config_tests:265: should_create_new_section...ok
config_tests:273: should_fail_to_set_binary_value...ok
[done in 0.055 s]
Config deletion tests
config_tests:277: should_return_undefined_atom_after_option_deletion...ok
config_tests:284: should_be_ok_on_deleting_unknown_options...ok
[done in 0.031 s]
Configs overide tests
config_tests:287: should_ensure_in_defaults...ok
config_tests:297: should_override_options...ok
config_tests:305: should_create_new_sections_on_override...ok
config_tests:313: should_win_last_in_chain...ok
[done in 0.058 s]
Config persistent changes
config_tests:316: should_write_changes...[0.003 s] ok
config_tests:330: should_ensure_that_default_wasnt_modified...ok
config_tests:338: should_ensure_that_written_to_last_config_in_chain...ok
[done in 0.038 s]
Test config with no files
config_tests:346: should_ensure_that_no_ini_files_loaded...[0.001 s] ok
config_tests:349: should_create_non_persistent_option...ok
config_tests:356: should_create_persistent_option...ok
[done in 0.025 s]
Test config_listener behaviour
config_tests:363: should_handle_value_change...ok
config_tests:368: should_pass_correct_state_to_handle_config_change...ok
config_tests:375: should_pass_correct_state_to_handle_config_terminate...ok
2015-11-02 08:31:55.693 [error] Undefined <0.2136.0> Supervisor config_sup had child undefined started with config:start_link(["/home/kxepal/projects/asf/couchdb/couchdb/tmp/etc/default_eunit.ini","/home/kxepal/projects/as...",...]) at <0.2137.0> exit with reason shutdown in context shutdown_error
config_tests:392: should_pass_subscriber_pid_to_handle_config_terminate...ok
2015-11-02 08:31:55.713 [error] Undefined <0.2183.0> Supervisor config_sup had child undefined started with config:start_link(["/home/kxepal/projects/asf/couchdb/couchdb/tmp/etc/default_eunit.ini","/home/kxepal/projects/as...",...]) at <0.2184.0> exit with reason shutdown in context shutdown_error
config_tests:401: should_not_call_handle_config_after_related_process_death...[0.001 s] ok
2015-11-02 08:31:55.732 [error] Undefined <0.2230.0> Supervisor config_sup had child undefined started with config:start_link(["/home/kxepal/projects/asf/couchdb/couchdb/tmp/etc/default_eunit.ini","/home/kxepal/projects/as...",...]) at <0.2231.0> exit with reason shutdown in context shutdown_error
config_tests:410: should_remove_handler_when_requested...[0.001 s] ok
2015-11-02 08:31:55.751 [error] Undefined <0.2277.0> Supervisor config_sup had child undefined started with config:start_link(["/home/kxepal/projects/asf/couchdb/couchdb/tmp/etc/default_eunit.ini","/home/kxepal/projects/as...",...]) at <0.2278.0> exit with reason shutdown in context shutdown_error
[done in 2.122 s]
[done in 2.926 s]
[done in 2.927 s]
[done in 2.933 s]
```
And not sure if these are for good.
---
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-config pull request: Don't start lager during setup for ea...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-173451479
Interestingly, I'm no longer able to reproduce the `[error] Undefined` errors, although I *am* seeing a lot more logging than before for some reason.
@kxepal, are you still getting those errors with this simplified patch? Or even why the logging has returned in test output?
```
(18.2.1)jay@mbp:~/proj/couchdb$ make eunit apps=config skip_deps+=couch_log,couch_epi
==> couch_epi (compile)
==> config (compile)
==> b64url (compile)
==> cassim (compile)
==> meck (compile)
==> couch_log (compile)
==> lager (compile)
==> couch_log_lager (compile)
==> chttpd (compile)
==> couch (compile)
==> couch_index (compile)
==> couch_mrview (compile)
==> couch_replicator (compile)
==> couch_plugins (compile)
==> couch_event (compile)
==> couch_stats (compile)
==> couch_peruser (compile)
==> ddoc_cache (compile)
==> ets_lru (compile)
==> fabric (compile)
==> bear (compile)
==> folsom (compile)
==> global_changes (compile)
==> goldrush (compile)
==> ibrowse (compile)
==> ioq (compile)
==> jiffy (compile)
==> khash (compile)
==> mango (compile)
==> mem3 (compile)
==> mochiweb (compile)
==> oauth (compile)
==> rexi (compile)
==> snappy (compile)
==> setup (compile)
==> rel (compile)
==> couchdb (compile)
==> couchdb (setup_eunit)
==> config (eunit)
======================== EUnit ========================
module 'config_writer'
module 'config_util'
module 'config_sup'
module 'config_listener'
module 'config_app'
module 'config'
config: to_integer_test...ok
config: to_float_test...ok
module 'config_tests'
CouchDB config tests
Config get tests
config_tests:220: should_load_all_configs...ok
config_tests:223: should_locate_daemons_section...ok
config_tests:226: should_locate_mrview_handler...ok
config_tests:230: should_return_undefined_atom_on_missed_section...ok
config_tests:234: should_return_undefined_atom_on_missed_option...ok
config_tests:238: should_return_custom_default_value_on_missed_option...ok
config_tests:242: should_only_return_default_on_missed_option...ok
config_tests:246: should_fail_to_get_binary_value...ok
config_tests:253: should_return_any_supported_default (for type(undefined))...ok
config_tests:253: should_return_any_supported_default (for type("list"))...ok
config_tests:253: should_return_any_supported_default (for type(true))...ok
config_tests:253: should_return_any_supported_default (for type(false))...ok
config_tests:253: should_return_any_supported_default (for type(0.1))...ok
config_tests:253: should_return_any_supported_default (for type(1))...ok
[done in 0.427 s]
Config set tests
config_tests:257: should_update_option...[notice] config: [log] level set to severe for reason nil
[0.012 s] ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
config_tests:264: should_create_new_section...[0.001 s] ok
config_tests:272: should_fail_to_set_binary_value...ok
[done in 0.079 s]
Config deletion tests
[notice] config: [log] level deleted for reason nil
config_tests:276: should_return_undefined_atom_after_option_deletion...ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
config_tests:283: should_be_ok_on_deleting_unknown_options...ok
[done in 0.046 s]
Configs overide tests
config_tests:286: should_ensure_in_defaults...ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
config_tests:296: should_override_options...ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
config_tests:304: should_create_new_sections_on_override...ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
config_tests:312: should_win_last_in_chain...ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
[done in 0.081 s]
Config persistent changes
[notice] config: [httpd] port set to 8080 for reason nil
config_tests:315: should_write_changes...[notice] config: [httpd] bind_address deleted for reason "8080"
[0.001 s] ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
config_tests:329: should_ensure_that_default_wasnt_modified...ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
config_tests:337: should_ensure_that_written_to_last_config_in_chain...ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
[done in 0.056 s]
Test config with no files
config_tests:345: should_ensure_that_no_ini_files_loaded...ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
[notice] config: [httpd] port set to 80 for reason nil
config_tests:348: should_create_non_persistent_option...ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
[notice] config: [httpd] bind_address set to 127.0.0.1 for reason nil
config_tests:355: should_create_persistent_option...ok
[notice] config_listener(couch_log_config_listener) stopped with reason: shutdown
[done in 0.038 s]
Test config_listener behaviour
config_tests:362: should_handle_value_change...ok
config_tests:367: should_pass_correct_state_to_handle_config_change...ok
config_tests:374: should_pass_correct_state_to_handle_config_terminate...ok
config_tests:391: should_pass_subscriber_pid_to_handle_config_terminate...ok
config_tests:400: should_not_call_handle_config_after_related_process_death...ok
config_tests:409: should_remove_handler_when_requested...ok
[done in 2.140 s]
[done in 2.868 s]
[done in 2.868 s]
[done in 2.893 s]
=======================================================
All 37 tests passed.
==> rel (eunit)
==> couchdb (eunit)
WARN: 'suites' command does not apply to directory /Users/jay/proj/couchdb/src/config
WARN: 'suites' command does not apply to directory /Users/jay/proj/couchdb/rel
WARN: 'suites' command does not apply to directory /Users/jay/proj/couchdb
WARN: 'tests' command does not apply to directory /Users/jay/proj/couchdb/src/config
WARN: 'tests' command does not apply to directory /Users/jay/proj/couchdb/rel
WARN: 'tests' command does not apply to directory /Users/jay/proj/couchdb
```
---
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-config pull request: Don't start lager during setup for ea...
Posted by jaydoane <gi...@git.apache.org>.
Github user jaydoane commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-153194830
@kxepal, I changed the approach to leave lager in (while removing other unnecessary deps), and just filtering nonexistent applications.
This patch no longer results in the error messages under CouchDB testing, and also runs downstream.
---
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-config pull request: Don't start lager during setup for ea...
Posted by kxepal <gi...@git.apache.org>.
Github user kxepal commented on the pull request:
https://github.com/apache/couchdb-config/pull/7#issuecomment-154493803
@jaydoane haha, thanks, it's better (: I'm still thinking how these changes correlates with yours in https://github.com/apache/couchdb-couch/pull/126 because it seems, again, explicit lager start isn't required. So this patch could be simplified. Theoretically.
---
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.
---