riak_core_info_service #898
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
riak_core_info_service*.erl files contains the solution.
Changed the code placement of where the result to give back to the dependent process happens. Now the Result is calculated in the riak_core_info_service_process callback_router function and then the handler is invoked.
- Add `state` record to `riak_core_info_service_process` to resolve issues with order of parameters. - Make children of `riak_core_info_service_sup` `permanent` rather than `temporary`
…tails. - Also, rename `get` handler to `invoke`
`handle_debug` on outbound messages.
|
Thanks @JeetKunDoug! Settings---
minimum_reviewers: 2
merge: true
build_steps:
- make clean
- make deps
- make compile
- make test
- make xref
- make dialyzer
org_mode: true
timeout: 1800 |
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
tburghart
reviewed
Feb 14, 2017
| @@ -0,0 +1,113 @@ | |||
| -module(riak_core_info_service_process). | |||
There was a problem hiding this comment.
At this point you've pretty much rolled your own gen_server, so it doesn't seem to me that the argument against using the OTP behavior has any basis left.
There was a problem hiding this comment.
I was thinking the same thing - simple gen_server with a few handle_info function heads should work just as well, and be less boilerplate code. I didn't know what the original reasoning was - maybe @macintux can weigh in and if he agrees we can just turn this into a gen_server process.
There was a problem hiding this comment.
The original "API" was leveldb sending custom Erlang messages that would not be routed via OTP. I felt strongly that implementing an OTP behavior would be effectively lying about the role the process played.
I haven't looked at the new code to see what's been done to it.
- Invoke the `shutdown` callback in `Mod:terminate`
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
tburghart
reviewed
Feb 14, 2017
| %% `gen_server' implementation | ||
|
|
||
| start_link(Registration, Shutdown, Source, ResponseHandler) -> | ||
| {ok, Pid} = gen_server:start_link(?MODULE, [Shutdown, Source, ResponseHandler], []), |
There was a problem hiding this comment.
Trivial, but why not just create the #state{} record here and pass it through intact?
There was a problem hiding this comment.
Probably just an "order of operations thing," as in, I morphed the code in such a way that this is how it ended up... good call though, will update.
rather than passing all the parameters down to `init`
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
JeetKunDoug
commented
Feb 14, 2017
src/riak_core_app.erl
Outdated
| InfoSource = {riak_core_bucket, get_bucket, []}, | ||
| ResultsHandler = {eleveldb_metadata, handle_metadata_response, []}, | ||
| {ok, _Pid} = riak_core_info_service:start_service(Registration, Shutdown, InfoSource, ResultsHandler), | ||
| ok. |
macintux
reviewed
Feb 15, 2017
| -behaviour(gen_server). | ||
|
|
||
| -record(state, { | ||
| source = undefined :: riak_core_info_service:callback(), |
There was a problem hiding this comment.
Suggest a different name; I first thought source was the source of the request. Perhaps service_provider?
There was a problem hiding this comment.
+1 - makes sense to me... will think about the name a bit but I like service_provider
There was a problem hiding this comment.
yea - go with service_provider unless you come up with something better - please make sure to rename things like the variables in riak_core_app.erl to match whatever you come up with.
|
I'm reasonably certain the whitelist for mapreduce will prevent this from being misused, but it's worth verifying my memory is correct. |
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
Also picked up a few spurious whitespace fixes.
Since `riak_core_info_service` provides such a detailed doc string, refer to it from the `_process` and `_sup` modules.
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
* Add paragraph breaks * Add section headers * Add cross-references * Add code markup * Add type documentation for `callback()` * Clarify usage docs
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
The name "source" was a bit confusing, so it has been renamed "provider" (as a binding name) or "service_provider" (as a record field). Also corrected the 3rd element of the invocation tuple: it's not a list of terms, it's just a term (which, of course, can be a list).
There seems to be an issue with build step **make_test** ! ☁️✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
⛔ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
|
Rather than do ugly stuff with I also caught some glitches along the way: dialyzer fix, create a shutdown callback for eleveldb. That last one will require some careful thought by someone not awake until 5am (i.e., not me). |
Add a test which throws an exception when receiving a response from riak_core and make sure we see a shutdown message.
There seems to be an issue with build step **make_test** ! ☁️✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
⛔ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
raghavkarol
reviewed
Feb 28, 2017
| apply_callback({Mod, Fun, StaticArgs}, CallSpecificArgs) -> | ||
| %% Invoke the callback, passing static + call-specific args for this call | ||
| erlang:apply(Mod, Fun, StaticArgs ++ CallSpecificArgs). | ||
| Args = StaticArgs ++ CallSpecificArgs, | ||
| _ = case (catch erlang:apply(Mod, Fun, Args)) of |
There was a problem hiding this comment.
with catch, a throw(...) in erlang:apply(Mod, Fun, Args) matches Result and is indistinguishable from a return value. If this is not the intention, a try-catch should be used instead.
There was a problem hiding this comment.
I think that's suitable. The code in here is pretty much the example I sent John, and I thought about that situation while throwing it together. If we were to use a try/catch, we'd effectively be saying a return by throw is an error, when it has plenty of uses as a simple break out of a loop.
tburghart
reviewed
Feb 28, 2017
| apply_callback({Mod, Fun, StaticArgs}, CallSpecificArgs) -> | ||
| %% Invoke the callback, passing static + call-specific args for this call | ||
| Args = StaticArgs ++ CallSpecificArgs, | ||
| _ = case (catch erlang:apply(Mod, Fun, Args)) of |
There was a problem hiding this comment.
_ = not needed because each case returns from the function
tburghart
reviewed
Feb 28, 2017
| _ = case (catch erlang:apply(Mod, Fun, Args)) of | ||
| {'EXIT', {Reason, _Stack}}=Exit-> | ||
| %% provider called erlang:error(Reason) | ||
| lager:warning("~p:~p/~B exited (~p)", |
There was a problem hiding this comment.
lager calls, otoh, can return structured results, so dialyzer will flag them when the option to check returns is on.
tburghart
approved these changes
Feb 28, 2017
|
Test failure is interesting. Also interesting that the status box on this PR doesn't make it obvious that there is a failure with the build: #898 (comment) The two new tests pass when running eunit on just the module locally, now trying eunit across entire project. Update: they fail locally as well, which will make this easier to fix. Please no one provide a 2nd approval yet. |
hazen
reviewed
Mar 1, 2017
| supervisor:start_link({local, ?SERVER}, ?MODULE, []). | ||
|
|
||
| start_service(Registration, Shutdown, Provider, Handler) -> | ||
| supervisor:start_child(?SERVER, [Registration, Shutdown, Provider, Handler]). |
There was a problem hiding this comment.
Is this a valid way to start a child? http://erlang.org/doc/man/supervisor.html#start_child-2
Here's my failed attempt at a child spec:
ChildSpec = {?SERVER, {riak_core_info_service_process, start_link, [Registration, Shutdown, Provider, Handler]}, permanent, 5000, worker, [riak_core_info_service_process]},
supervisor:start_child(?SERVER, ChildSpec).
There was a problem hiding this comment.
simple_one_for_one supervisors handle child specs differently. See the 4th paragraph in that section of the document.
These tests were failing when invoked as part of a larger suite due to unrelated messages arriving from earlier tests. * Flush the mailbox during setup * Be more selective about which messages we process * Add more comments * Refactor away some boilerplate redundancy
|
Testing problem identified, fixed. Also some other cleanup on the tests. Ready for re-review. |
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
hazen
reviewed
Mar 1, 2017
src/riak_core_info_service.erl
Outdated
| _ -> | ||
| %% Launching and terminating the supervisor is an async operation with | ||
| %% all the non-determinism that that implies. So, we wait. | ||
| sup_wait(Name, Fun) -> |
|
+1 |
* The shutdown callback always takes the info service process pid being shut down as an argument, so create a new function to take that pid * Tell eleveldb the pid is no longer in service (corresponding function is already in the appropriate branch for review, MvM is working on the NIF) * The supervisor will restart the process with the callbacks, including registration, so don't try to re-register after shutdown
There seems to be an issue with build step **make_xref,make_dialyzer** ! ☁️✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
⛔ MAKE_XREF
⛔ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
tburghart
reviewed
Mar 3, 2017
src/riak_core_app.erl
Outdated
| @@ -152,8 +152,11 @@ register_capabilities() -> | |||
| %% TODO: This belongs in riak_kv - an issue will be created to move it, but time constraints. | |||
| start_eleveldb_info_service() -> | |||
| Registration = {eleveldb, set_metadata_pid, []}, | |||
| Shutdown = {?MODULE, start_eleveldb_info_service, []}, | |||
| Shutdown = {?MODULE, shutdown_eleveldb_info_service, []}, | |||
There was a problem hiding this comment.
Changing this to {eleveldb, remove_metadata_pid, []} should shut xref and dialyzer up, with the added benefit of being consistent with registration and eliminating the separate shutdown_eleveldb_info_service/1 function altogether.
|
xref and dialyzer are unhappy because Thumbs doesn't support multiple unnerved branches yet. Both pass locally using the proper eleveldb branch.
I agree it's reasonable to remove the indirection anyway. Will do.
…Sent from my iPad
On Mar 3, 2017, at 7:39 AM, Ted Burghart ***@***.***> wrote:
@tburghart commented on this pull request.
In src/riak_core_app.erl:
> @@ -152,8 +152,11 @@ register_capabilities() ->
%% TODO: This belongs in riak_kv - an issue will be created to move it, but time constraints.
start_eleveldb_info_service() ->
Registration = {eleveldb, set_metadata_pid, []},
- Shutdown = {?MODULE, start_eleveldb_info_service, []},
+ Shutdown = {?MODULE, shutdown_eleveldb_info_service, []},
Changing this to {eleveldb, remove_metadata_pid, []} should shut xref and dialyzer up, with the added benefit of being consistent with registration and eliminating the separate shutdown_eleveldb_info_service/1 function altogether.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Looks good! 👍✅ MERGE
✅ MAKE_CLEAN
✅ MAKE_DEPS
✅ MAKE_COMPILE
✅ MAKE_TEST
✅ MAKE_XREF
✅ MAKE_DIALYZER
⬜ 0 of 2 Code reviews from organization basho |
|
+1 9eda941 |
raghavkarol
approved these changes
Mar 3, 2017
There was a problem hiding this comment.
LGTM - I suggest using common test instead of eunit for tests that need fixture setup. Each common test case is a separate process, so flushing messages is not required. Also, as of now, rebar3 doesn't have an options to run a single eunit test case (rebar does), while it is possible to do this for ct
|
+1 |
✅ 2 of 2 Code reviews from organization basho
|
|
Merging and closing this pr |
|
Successfully merged basho/riak_core/pulls/898 (9eda941 on to develop) ---
:sha: 3691ac4c31fbd89b2742c8d1624ca64a2715f472
:merged: true
:message: Pull Request successfully merged
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements the
riak_core_info_serviceset of processes - a supervisor and one or more children that are created to support NIFs that need to call in to Riak to get information.riak_core_info_service- APIriak_core_info_service_sup- supervisor forriak_core_info_service_processprocesses.riak_core_info_service_process- a lightweight process that can respond to a specified request by invoking a function and calling a callback to return the results.See
riak_core_app.erlfor an example of registering and starting a service.TODO:
eleveldbservice registration is acceptable, or if we want to use a more indirect method of registering the callbacks in this case (cuttlefish schema perhaps? Do we have other ways of doing this that don't requireeleveldbto directly call in toriak_core?)riak_core_info_service_processagen_serverat this point.