[
https://issues.apache.org/jira/browse/HDFS-10441?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15295860#comment-15295860
]
James Clampffer commented on HDFS-10441:
----------------------------------------
Thanks for the really detailed review Bob! I'll have a new patch up tomorrow
that should cover most of your comments, let me know if my responses sound in
line with what you'd like.
bq. Should Options::ha_namenodes_ be a map<string,HANamenodeInfo> to make it
easier to look up?
Yes, I just implemented this because the first round only dealt with 1
nameservice. The plan is to have a map<string,vector<>> of nameservice IDs to
namenodes in that service. Then add a getter that can look up NNs by service
or default to whatever is specified by defaultFS.
bq. Rather than have a server_exception_type, I would like to see the explicit
error types as peers to our other error types. The fact that it came from a
Java exception on the server side is an implementation detail of the
communications. The kExceptionType should really be an "UnexpectedException"
type, and all of the others should be mapped to semantically informative types
(using an awesome lookup table, like you did)
Agreed. This was partly because it let me make the assumption that if it
happened on the server side it was probably a good time to fail over. Partly
because I forgot how c++ enums worked.. I was concerned about having the int
value of the enum entry collide with the std::errc codes but the compiler will
deal with that. Would you be opposed to a method that would determine if an
error likely originated on the server side?
bq. Not super-critical, but it's a little bit cleaner and (possibly) faster to
use strstr in HDFSConfiguration::SplitOnComma. Once you know the start and end,
you can use std::string::string(const char *, int) to construct the string.
Thanks for the reference, I hadn't used that. I'll fix this in an upcoming
patch(little lower priority than the functional recommendations).
bq. Instead of trying to manually parse the URI in ParsePartialURI, would it be
easier to do a quick check for "://", and if it's not there, prepend
DEFAULT_SCHEME (hdfs://), then pass it to the full-featured URI parser?
Most likely. For now I'll go down that path. I've also filed HDFS-10411
because when the uri library fails to parse an incomplete url it will also jump
on an ininitialized value according to valgrind. I'll post a more general fix
to the parser in HDFS-10411.
bq. By design the Configuration class is really a stupid container. If we can,
all the logic should be in the HDFSConfigurationLoader.
That sounds like a much better design than what I have, will do.
bq. It seems that the nameservice lookups aren't actually URIs; they're just
server:port addresses. Perphaps rather than storing a URI object, the
HANamenodeInfo should just have a server and port field. The rest of the URI
semantic space (scheme, user, password, path, fragment, etc.) really doesn't
apply here, and would be an error if it appeared in the config file.
I agree. I was hoping to avoid adding a new object to handle this case but
with the trouble it's giving the parser it'd probably be worth it.
bq. I would prefer "FormatURI" or "ToString" over "FmtURI" as a method name
Yep, will do. FmtURI was a quick hack that I should have fixed up before
posting.
bq. I've noticed that we've grown used to "if (!stat.ok) { LOG_XXX(..., <Some
fixed string>)". In trying to debug problems, I'm unhappy with me for not
including the error message. As you touch any logging messages, << out the
status.ToString() also
I agree. The log messages on this last patch were a mess (thanks for the
patience in reviewing). I'll include status::ToString in future messages;
perhaps the logger should take a status object as an argument to enforce this.
bq.As an extension of that: LOG_WARN(kRPC, << "Tried to connect to standby!");
What node was I trying to connect to?
I'll clean this up.
bq. EnqueueRequestsWithLock: I think elsewhere in the codebase we have used
EnqueueRequests_locked; let's keep that standard
Didn't see that, sounds like what I was looking for, I'll switch.
bq. is_locked: we already have a lock_held method in util.h (that works with
reentrant mutexes, too)
Yea, I'll switch to that. I had the patch from HDFS-10409 applied and it made
it into here.
bq. I know we're going to add more-than-one HA NN later, but a good idiom is to
keep them in a sorted vector, and have an index pointing to the current one.
Then make next = (curr + 1) % size.
Sounds good. That's only marginally more complicated than the std::swap but a
lot more general.
bq. Name lookup in ResoloveAllEndpoints needs to be async. This can be
follow-up if necessary.
I agree. I'd like to hold off on this in order to keep HA as simple as
possible until it's well tested.
bq. In general, having a recursive_mutex (HA NN Tracker) is a bad smell that
we're not thinking about concurrency tightly enough. I'm not sure that's the
case here, but let's think about it.
Agreed. I'll be revisiting this. IIRC it was used so that I could have
getters that worked in the locked and unlocked contexts.
bq. Make sure to clean up TODOs and anything referring to internal server names
The internal server references are a bit embarrassing. I need to make my
precommit check a little more rigorous.
bq. Unset port should be a WARNING, if we're going to default and move on
Sounds good to me.
bq. In RpcEngine::Start, if resolution fails and we clear out
ha_persisted_info_, what will happen when we run?
The idea was that that would be part of a check that would determine if the
client was allowed to failover, so it should always have an
if(ha_persisted_info_) prior to reading. If not it's supposed to default to a
non HA config. I'm going to update this with some sort of is_rpc_enabled()
getter so it can take other things into account as well.
bq. Better logging than "----" and "Caught this early"
Yep. Again thanks for looking at a patch that wasn't cleaned up as well as it
should have been before posting for review.
bq. In RpcEngine::RpcCommsError, we should try for symmetry in retry count and
failure count. Let's pull the IncrementRetryCount out of being inline, and just
use both getters in the call to ShouldRetry.
The inline was just a hack so I could leave it in the header during
development, will move out and push the getters into ShouldRetry.
bq. You have a comment for eventually failing out messages if we're bouncing
between standby nodes, but it looks like we only actually check if it's not a
kStandbyException. If I'm mis-reading, let me know.
You're reading is correct, I forgot to update that comment to reflect a batch
of changes I made. I'll pass through and make sure the other comments are in
good shape too.
bq. We don't need the first line here, do we?
+ last_endpoints_.clear();
+ last_endpoints_ = new_active_nn_info.endpoints;
Not at all. I stripped some stuff out before submitting and forgot to get rid
of the clear(). Good catch.
bq. In RpcConnection::HandleRpcResponse, I think we should be checking status
and erroring out before calling RemoveFromRunningQueue. We should look at this
closely, and then we could get rid of the EnqueueReqeustsWithLock method.
Yea, I need to revisit this, that's one of the trickier bits of the RPC code
IMO. I remember there being some reason why I implemented it that way, but I'm
not sure if that was just an intermediate step I was doing for a personal
development milestone.
> libhdfs++: HA namenode support
> ------------------------------
>
> Key: HDFS-10441
> URL: https://issues.apache.org/jira/browse/HDFS-10441
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Reporter: James Clampffer
> Attachments: HDFS-10441.HDFS-8707.000.patch
>
>
> If a cluster is HA enabled then do proper failover.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
