hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Todd Lipcon (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-2982) Startup performance suffers when there are many edit log segments
Date Sat, 19 May 2012 00:22:08 GMT

    [ 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

        

Mime
View raw message