[
https://issues.apache.org/jira/browse/HDFS-2982?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13279358#comment-13279358
]
Todd Lipcon commented on HDFS-2982:
-----------------------------------
- in BookKeeperJournalManager, getInputStream and getNumberOfTransactions can
be made package-private now, right, since they aren't part of the JM API
anymore?
EditLogFileInputStream:
- in init(), add Preconditions.checkState(state == State.UNINIT);
- I would expect to see state = State.OPEN at the end of init(), not at its
call site
- in {{nextOpImpl}}, combine the two catch clauses into a {{catch (Throwable
t)}} to avoid the duplicate code. You can use the {{Throwables}} class from
Guava to propagate the exception without too much messy code.
----
{code}
+ if (!resync) {
+ throw e;
+ }
+ return null;
{code}
- I think it's clearer to invert the logic here:
{code}
if (resync) {
return null;
}
throw e;
{code}
since resync is the more unusual case, whereas rethrowing is the usual.
Also, I think the {{resync}} parameter would be better named
{{treatErrorsAsEof}} or something -- it's not actually resynchronizing, it's
just ignoring a single error and treating it like there being no more valid ops.
----
{code}
+ LOG.info("skipping the rest of " + this + " since we " +
+ "reached txId " + txId);
+ long skipAmt = file.length() - tracker.getPos();
+ if (skipAmt > 0) {
{code}
This will get printed on every log, right? I think we should only log if
skipAmt > 0.
----
{code}
+ return nextOpImpl(true);
} catch (IOException e) {
+ LOG.error("nextValidOp: got exception while reading " + this, e);
+ return null;
+ } catch (RuntimeException e) {
+ LOG.error("nextValidOp: got exception while reading " + this, e);
{code}
can you combine the catch clauses, same as above?
----
{code}
+ LOG.error("got IOException while trying to validate header of " +
+ elf + ". Skipping.", e);
{code}
----
{code}
+ if (recovery == null) {
+ closeAllStreams(streams);
+ throw e;
+ }
+ LOG.error(e);
{code}
Same as above - I think it is easier to understand as:
{code}
if (recovery != null) {
LOG.error(e);
// and continue
} else {
closeAllStreams(streams);
throw e;
}
{code}
----
{code}
+ // This code will go away as soon as RedundantEditLogInputStream is
+ // introduced.
{code}
Worth adding a reference to the JIRA here so people can find what you're
talking about in the interim.
----
{code}
+ // We don't want to throw an exception from here, because that would make
+ // recovery impossible even if the user requested it.
{code}
This comment makes more sense inside the {{catch}} clause. Also should add
something like: {{an exception will be thrown later when actually applying
edits, since we verify txids for each operation}}
----
{code}
+ txId = elis.getLastTxId() + 1;
{code}
can you add an {{assert elis.getLastTxId() > 0}} here or some other sanity
check to make sure we didn't accidentally get an in-progress segment?
----
- I'm still unconvinced that the changes to validateEditLog belong here. They
might be good changes, but they don't have anything to do with the performance
fix. Your justification above was about why the changes are important for
recovery mode, not why they're important for getting rid of the n^2 behavior of
startup, which is what this JIRA is about.
- Similarly, the other changes in FSEditLogLoader.java (regarding the treatment
of {{logVersion}}) don't seem to have anything to do with this change.
----
{code}
+ EDIT_LOG_INPUT_STREAM_COMPARATOR = new Comparator<EditLogInputStream>() {
+ @Override
+ public int compare(EditLogInputStream a, EditLogInputStream b) {
+ long fa = a.getFirstTxId();
+ long fb = b.getFirstTxId();
+ if (fa < fb) {
+ return -1;
+ } else if (fa > fb) {
+ return 1;
+ }
+ long la = a.getLastTxId();
+ long lb = b.getLastTxId();
+ if (lb < la) {
+ return -1;
+ } else if (lb > la) {
+ return 1;
+ }
+ return a.getName().compareTo(b.getName());
+ }
+ };
{code}
Use {{ComparisonChain}} from Guava here - much easier to read.
----
- The algorithm in {{JournalSet.selectInputStreams}} could do with some
explanation in an inline comment.
- {{testManyEditLogSegments}} still missing the {{@Test}} annotation.
- {{prepareUnfinalizedTestEditLog}} needs javadoc - especially given it has an
"out-parameter" - it's not obvious what it does without doc
- the "out-parameter" in fact doesn't appear to be used at all..
----
{code}
+ * @return The number of transactions
+ * @throws IOException
+ */
+ static long getNumberOfTransactions(FileJournalManager jm, long fromTxId,
+ boolean inProgressOk, boolean abortOnGap) throws IOException {
{code}
this function doesn't seem to close its streams (I see it's moved code, but may
as well fix this while you move it)
----
- what's the reasoning of the changes in TestNameNodeRecovery?
> Startup performance suffers when there are many edit log segments
> -----------------------------------------------------------------
>
> Key: HDFS-2982
> URL: https://issues.apache.org/jira/browse/HDFS-2982
> Project: Hadoop HDFS
> Issue Type: Bug
> Components: name-node
> Affects Versions: 2.0.0
> Reporter: Todd Lipcon
> Assignee: Colin Patrick McCabe
> Priority: Critical
> Attachments: HDFS-2982.001.patch, HDFS-2982.002.patch,
> HDFS-2982.003.patch, HDFS-2982.004.patch, HDFS-2982.005.patch
>
>
> For every one of the edit log segments, it seems like we are calling
> listFiles on the edit log directory inside of {{findMaxTransaction}}. This is
> killing performance, especially when there are many log segments and the
> directory is stored on NFS. It is taking several minutes to start up the NN
> when there are several thousand log segments present.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
