[
https://issues.apache.org/jira/browse/HDFS-4979?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13710186#comment-13710186
]
Chris Nauroth commented on HDFS-4979:
-------------------------------------
Hi, Suresh. The approach in the patch is looking good so far. I still need to
dig into the trickier logic for {{startFile}} and {{appendFile}}, but here are
some initial comments.
# I noticed that we are choosing not to cache failure responses. I expect this
is fine, but I thought I'd mention it and see if you agree. It does mean that
some side effects could happen multiple times in a retry. One example is the
audit log. For example, assume a {{FSNamesystem#renameTo}} call where the
client does not have the required permissions. Assume this call gets retried.
The response to each try is the same, but there would be 2 lines in the audit
log for a single logical call. I expect side effects like this are acceptable,
because our real goal is graceful recovery of clients during a failover, and
clients don't see these side effects in the RPC responses.
# This block of code (and similar variants) get repeated a few times, so
perhaps there is a refactoring opportunity:
{code}
if (retryCache.isRetriedRequest()) {
readLock();
try {
if (retryCache.inRetryCache()) {
return;
}
} finally {
writeUnlock();
}
}
{code}
# I have a question on {{FSNamesystem#renameToInt}}:
{code}
status = renameToInternal(pc, src, dst);
if (status) {
resultingStat = getAuditFileInfo(dst, false);
}
} finally {
if (status) {
retryCache.add();
}
writeUnlock();
}
getEditLog().logSync();
if (status) {
logAuditEvent(true, "rename", src, dst, resultingStat);
}
return status;
{code}
The finally block uses the {{status}} flag to check success and save to the
retry cache. However, the subsequent calls to {{getAuditFileInfo}} and
{{logAuditEvent}} could throw {{IOException}}. This means the first try could
get a failure response, and then retries would get a success response out of
the retry cache. Is this acceptable, or do we need to move the cache save
lower in the method after all operations that could throw?
# We need to watch out for one cacheable method calling another. One example I
see is {{startFileInternal}} calling {{delete}} if overwrite option is used.
The {{delete}} method caches its result, and it doesn't know the difference
between a client RPC call and an internal call. This could cause some strange
behavior for clients. For example, assume a {{create}} call with overwrite
that succeeds in the {{delete}} but then fails in a later step. This would
leave a success response in the cache (for the successful delete), so if the
client retries, it would receive a success response even though the file was
not created.
# {{deleteInternal}} saves to the retry cache before processing
{{collectedBlocks}} and {{removedINodes}}. Is that correct? At first glance,
this seems fine, because the methods called do not throw checked exceptions.
However, {{deleteSnapshot}} has similar logic, and it waits until after
handling {{collectedBlocks}} and {{removedINodes}} before saving to the cache.
Should these 2 methods be consistent?
{quote}
I noticed that where I am setting boolean success to true in try blocks is
susceptible to issues related to reordered writes. I am going to change them to
AtomicBoolean in my next patch.
{quote}
I expect this won't be necessary, because the success tracking flags that you
used are local variables, and therefore only accessible from a single thread.
If they were shared memory accessible from multiple threads, then reordering
would be a problem.
http://docs.oracle.com/javase/specs/jls/se7/html/jls-17.html#jls-17.4.1
{quote}
Local variables (§14.4), formal method parameters (§8.4.1), and exception
handler parameters (§14.20) are never shared between threads and are unaffected
by the memory model.
{quote}
> Implement retry cache on the namenode
> -------------------------------------
>
> Key: HDFS-4979
> URL: https://issues.apache.org/jira/browse/HDFS-4979
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: namenode
> Reporter: Suresh Srinivas
> Assignee: Suresh Srinivas
> Attachments: HDFS-4979.1.patch, HDFS-4979.2.patch, HDFS-4979.patch
>
>
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira
