Attached patch , Please review ..

Akira Ajisaka Would you please review ?

Good catch and thanks Surendra Singh Lilhore for taking this issue. Here are three comments.
1. StringBuffer should be StringBuilder. StringBuilder is faster.
2. You don't need to use StringBuffer except in CreateEvent and MetadataUpdateEvent. For example,

+    @Override
+    public String toString() {
+      StringBuffer content = new StringBuffer();
+      content.append("UnlinkEvent [path=" + path + ", timestamp=" + timestamp
+          + "]");
+      return content.toString();
+    }

can be simplified as

+    @Override
+    public String toString() {
+      return new String("UnlinkEvent [path=" + path + ", timestamp=" + timestamp
+          + "]");
+    }

3. Would you add null check for perm in MetadataUpdateEvent#toString()? We should add null check for all the optional variables. inotify.proto tells us whether the variable is required or not.

+    @Override
+    public String toString() {
+      return new String("UnlinkEvent [path=" + path + ", timestamp=" + timestamp
+          + "]");
+    }

new String is redundant and should be removed. Sorry for the confusion.

Thanks Akira Ajisaka for review and comments.

We should add null check for all the optional variables. inotify.proto tells us whether the variable is required or not.

I think only for string optional variable we should add check

I Attached new patch, Please review...

-1 overall

This message was automatically generated.

I think this is a good improvement. I would caution people not to start parsing the toString representation since it might change over time (if we add new optional fields to the Events). But it is nice for debugging and logging.

Thanks Surendra Singh Lilhore for updating the patch. Three comments:
1. I still see some unnecessarily output in the test (modified to log the events). mtime=0, atime=0, replication=0 is confusing to me.

2015-07-22 17:49:45,769 [Thread-0] INFO  hdfs.TestDFSInotifyEventInputStream (TestDFSInotifyEventInputStream.java:testBasic(289)) - MetadataUpdateEvent [path=/dir, metadataType=PERMS, mtime=0, atime=0, replication=0, perms=rw-rw-rw-, xAttrsRemoved=false]
2015-07-22 17:49:45,770 [Thread-0] INFO  hdfs.TestDFSInotifyEventInputStream (TestDFSInotifyEventInputStream.java:testBasic(302)) - MetadataUpdateEvent [path=/dir, metadataType=OWNER, mtime=0, atime=0, replication=0, ownerName=username, groupName=groupname, xAttrsRemoved=false]

Can we switch by metadataType and output the necessarily information as follows?

@Override
public String toString() {
  StringBuilder content = new StringBuilder();
  content.append("MetadataUpdateEvent [path=" + path + ", metadataType="
      + metadataType);
  switch (metadataType) {
  case TIMES:
    content.append(", mtime=" + mtime + ", atime=" + atime);
    break;
  case REPLICATION:
    content.append(", replication=" + replication);
    break;
  ...
  }
  content.append(']');
  return content.toString();
}

2. I'm thinking logging the event in the test is good for debugging.

      Assert.assertTrue(re.toString().contains("RenameEvent [srcPath="));

3. (minor) re.toString().startsWith is more strict. It should be used instead of String.contains.

       Event.CloseEvent ce3 = (Event.CloseEvent) batch.getEvents()[2];
       Assert.assertTrue(ce3.getPath().equals("/file2"));
       Assert.assertTrue(ce3.getTimestamp() > 0);
+      Assert.assertTrue(ue2.toString().contains("UnlinkEvent [path="));

4. (minor) Would you move the assertion before defining ce3?

—

I found some assertions are wrong in the test. Here is an example.

      Assert.assertTrue(batch.getEvents()[0].getEventType() == Event.EventType.RENAME);
      Event.RenameEvent re3 = (Event.RenameEvent) batch.getEvents()[0];
      Assert.assertTrue(re3.getDstPath().equals("/dir/file5"));
      Assert.assertTrue(re3.getSrcPath().equals("/file5"));
      Assert.assertTrue(re.getTimestamp() > 0);

We should fix them in a separate jira.

Thanks Colin P. McCabe for the comment.

I would caution people not to start parsing the toString representation since it might change over time (if we add new optional fields to the Events). But it is nice for debugging and logging.

Adding @Public and @Unstable to the subclasses is good for you? Event class is annotated, but the subclasses are not annotated and that's why they don't exist in API doc.

Good idea... I think the subclasses should have a @Public annotation. I think all the functions are "stable" except for toString.

I think the subclasses should have a @Public annotation. I think all the functions are "stable" except for toString.

Agree. Hi Surendra Singh Lilhore, would you add @Public annotation to the subclasses and @Unstable annotation to toString()?

Thanks Akira Ajisaka and Colin P. McCabe for comments.

I attached new patch. Please review ....

I found some assertions are wrong in the test. Here is an example.

I raised new jira for fixing assertions in test class..HDFS-8810

-1 overall

This message was automatically generated.

Attached new patch.

Fixed checkstyle error and removed whitespace...

-1 overall

This message was automatically generated.

Thanks Surendra for updating the patch. Mostly looks good to me. Would you add @Public annotation to CloseEvent class?

Sorry I missed it.. I updated the patch

Please review ...

-1 overall

This message was automatically generated.

+1, the test failures looks unrelated to the patch. The tests passed locally.

Committed this to trunk and branch-2. Thanks Surendra Singh Lilhore and Colin P. McCabe for the contribution.

FAILURE: Integrated in Hadoop-trunk-Commit #8216 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8216/)
HDFS-8735. Inotify: All events classes should implement toString() API. Contributed by Surendra Singh Lilhore. (aajisaka: rev f8f60918230dd466ae8dda1fbc28878e19273232)

• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSInotifyEventInputStream.java
• hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/inotify/Event.java

Thanks Akira Ajisaka for review and commit..

Thanks, Surendra Singh Lilhore and Akira Ajisaka.

FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #267 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/267/)
HDFS-8735. Inotify: All events classes should implement toString() API. Contributed by Surendra Singh Lilhore. (aajisaka: rev f8f60918230dd466ae8dda1fbc28878e19273232)

• hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/inotify/Event.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSInotifyEventInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

FAILURE: Integrated in Hadoop-Yarn-trunk #997 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/997/)
HDFS-8735. Inotify: All events classes should implement toString() API. Contributed by Surendra Singh Lilhore. (aajisaka: rev f8f60918230dd466ae8dda1fbc28878e19273232)

• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/inotify/Event.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSInotifyEventInputStream.java

FAILURE: Integrated in Hadoop-Hdfs-trunk #2194 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2194/)
HDFS-8735. Inotify: All events classes should implement toString() API. Contributed by Surendra Singh Lilhore. (aajisaka: rev f8f60918230dd466ae8dda1fbc28878e19273232)

• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSInotifyEventInputStream.java
• hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/inotify/Event.java

FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #256 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/256/)
HDFS-8735. Inotify: All events classes should implement toString() API. Contributed by Surendra Singh Lilhore. (aajisaka: rev f8f60918230dd466ae8dda1fbc28878e19273232)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSInotifyEventInputStream.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/inotify/Event.java

SUCCESS: Integrated in Hadoop-Mapreduce-trunk-Java8 #264 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/264/)
HDFS-8735. Inotify: All events classes should implement toString() API. Contributed by Surendra Singh Lilhore. (aajisaka: rev f8f60918230dd466ae8dda1fbc28878e19273232)

• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSInotifyEventInputStream.java
• hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/inotify/Event.java
• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt

SUCCESS: Integrated in Hadoop-Mapreduce-trunk #2213 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2213/)
HDFS-8735. Inotify: All events classes should implement toString() API. Contributed by Surendra Singh Lilhore. (aajisaka: rev f8f60918230dd466ae8dda1fbc28878e19273232)

• hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
• hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/inotify/Event.java
• hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDFSInotifyEventInputStream.java