Feature/az/api entrypoints coverage #1084
Conversation
| ]). | ||
|
|
||
| -record(ctx, { | ||
| api_version, %% integer() - Determine which version of the API to use. |
There was a problem hiding this comment.
Is there a reason you are not specifying the types in the signature instead of just in a comment? For example: api_version :: integer().
There was a problem hiding this comment.
That's actually from my PR... but was keeping consistency w/ https://github.com/basho/riak_kv/blob/develop/src/riak_kv_wm_object.erl#L157 and other files. The better question is why we've been doing that I reckon.
There was a problem hiding this comment.
@cmeiklejohn @hmmr I've made an update to https://github.com/basho/riak_kv/blob/develop/src/riak_kv_wm_object.erl#L157 that updates types for wm_preflist. Still relies on riak_core pr -> https://github.com/basho/riak_core/pull/705/files (for funs and types).
There was a problem hiding this comment.
Actually, I have no function that takes a preflist as an argument or returns one, so I should be fine. It's good to know where those types are, however.
An unrelated question: in newly written code, there is only one api_version I have to deal with (the current one). Do I still need to future-proof it by introducing api_version into my ctx record?
hmmr
force-pushed
the
feature/az/api-entrypoints-coverage
branch
from
2ea87e9
to
d5feabb
Compare
|
It still merges cleanly, so no changes; just a rebase onto HEAD of develop. Failed due to required bits (basho/riak_pb#106) still awaiting approval. |
hmmr
force-pushed
the
feature/az/api-entrypoints-coverage
branch
from
d5feabb
to
43176af
Compare
|
I disagree generally with the approach. We already have preflist calculation as an API request, why not simply provide the nodename + host/port/protocol for every interface? The client can cache the network information and simply request the preflist for the desired bucket-type/bucket/key, and then match up the nodenames to existing connections or new connections to be opened. Obviously, the client should also periodically re-request for the live nodes' interfaces, or by be triggered some signal from the server. Also, I'm doubful about how you're deriving the host interface information, because it is totally valid for a listener to start up with |
|
@seancribbs The issue you point out concerning listening ip where the external ip is meant, is now quite embarrassingly obvious to me. It's a blunder I've just committed (no, I have sat on it feeling good for a week!). What's needed here is some code path eventually calling I will review the whole thing tomorrow and report back. |
There are accompanying changes in newly created branches:
* riak_pb/feature/az/api-entrypoints-coverage, and
* riak_api/feature/az/api-entrypoints-coverage
that will have to be merged first.
The following HTTP requests are accepted (values for parameter P are
"http", "pbc", assuming the latter by default):
1) /ring/coverage/bucket/B/key/K?proto=P
returning a JSON of the form:
[{Host:[Port]}]
indicating Host:Port is where an entry point is for optimal data access
to the key K in bucket B, via the given protocol API.
2) /ring/coverage?proto=P
returning a JSON of the form:
[{Host:[Port]}]
providing a complete API entry point map for access to the entire
cluster via that protocol.
HTTP responses are cached until the next multiple of (a default of) 15 secs,
configurable via parameter 'ring_coverage_cache_expiry_time'.
For pb, two new messages are provided:
RpbApiEpReq (code 90),
RpbApiEpResp(code 91),
RpbApiEpMapReq (code 92),
RpbApiEpMapResp(code 93),
with the relevant snippet in riak_pb/src/riak_kv.proto:
enum RpbApiProto {
pbc = 0;
http = 1;
}
message RpbApiEp {
required bytes host = 1;
repeated int32 port = 2;
}
message RpbApiEpReq {
required bytes bucket = 1;
required bytes key = 2;
optional RpbApiProto proto = 3 [default = pbc];
}
message RpbApiEpResp {
repeated RpbApiEp eplist = 1;
}
message RpbApiEpMapReq {
optional RpbApiProto proto = 1 [default = pbc];
}
message RpbApiEpMapResp {
repeated RpbApiEp eplist = 1;
}
The coverage information is obtained in two steps:
1. using riak_core_apl:get_apl_ann(), get active preflist for given
Bucket and Key, and extract riak nodes from it;
2. make an rpc call on those nodes to:
(a) determine which of the relevant listener apps (i.e., riak_api_pb_listener,
riak_api_web) are running on those nodes,
and find the ports these are listening on.
(b) get the bound ip address of the first non-loopback interface on the
underlying os.
The corresponding new riak python client methods are
* client.get_api_entry_point(bucket, key, proto),
* client.get_api_entry_points_map(proto)
included in
github.com/basho/riak-python-client/tree/feature/az/api-entrypoints-coverage
|
Reworked, now the functions return the proper external ip instead of 127.0.0.1. @seancribbs concerning your suggestion to return the nodename and host/port, much as I'd like to just go with that, the java guys seem to only want the external ip and the port(s), not the nodenames proper or the part after '@' in those. I've been scratching my head hard, but other than calling getifaddrs on location, I just don't see how one can figure out the ip to which external clients can connect. I'll be happy to stand corrected, but at any rate, the scope of DP-6 is precisely about that. I grepped the entire riak/deps/ and found no matches on getifaddr (except an odd one in erlang_js). I am provisionally force-pushing the amended commit, for @srgg to continue his work on riak java client for the Thursday demo. There are a couple of issues to resolve, notably what 'heuristics' to apply in the case of multihomed hosts (such as those with infiniband in addition to plain eth0). |
hmmr
force-pushed
the
feature/az/api-entrypoints-coverage
branch
from
43176af
to
99613c9
Compare
|
I suppose this is fine for a demo, but before this merges, I'd like to see the following changes:
|
|
To go the whole hog, I now see I'll have to:
Is that a plan? |
|
|
@seancribbs Thanks for commenting and dropping precious hints. I. My line of reasoning was about this call: https://github.com/basho/riak_kv/blob/develop/src/riak_kv_web.erl#L115. It returns (on my devrel setup):
where I expect these new properties to appear: II. Indeed, let it be triggered by a dropped connection (and failure to establish one initially, too). Then, when a client times out or sees it gone, it will re-request the entire entry points map, possibly with &force_refresh=1 (or some such) to cause it to be regenerated. And then no periodic autorefresh is necessary. III. Suppose riak.conf initially has, e.g, My proposition is to check whether the configured listening address, unless it is 0.0.0.0, does match the bound address on some ethX in the system, and take steps to correct the situation. IV. It's simply the complete list of all API entry points into the cluster. The idea is to extend this list with details on what key ranges are quickest accessed via each entry point -- and thus attain the meaning of "coverage". Properly naming things goes a long way, indeed. |
|
Another reworking effort to address @seancribbs latest comments. Due to massive changes over what went into this branch, I pushed the new work into https://github.com/basho/riak_kv/tree/feature/az/api-entrypoints-coverage-rebased. |
|
Work and further discussions moved to #1087; closing this one then. |
This PR depends on basho/riak_pb#106, in turn depending on the bunch of PRs by @zeeshanlakhani.