Using BufferedFsInputStream to wrap FSInputStream for FSDataInputStream #373

n3nash · 2018-04-03T05:54:03Z

This PR is not ready for any review.

n3nash · 2018-04-05T06:00:02Z

hoodie-client/src/test/java/com/uber/hoodie/common/HoodieTestDataGenerator.java

@@ -69,7 +70,7 @@ public HoodieTestDataGenerator(String[] partitionPaths) {
  }

  public HoodieTestDataGenerator() {
-    this(new String[] {"2016/03/15", "2015/03/16", "2015/03/17"});
+    this(new String[]{"2016/03/15", "2015/03/16", "2015/03/17"});


Not sure why this is happening, my reformat does this.

n3nash · 2018-04-05T06:00:58Z

hoodie-common/src/main/java/com/uber/hoodie/common/table/log/HoodieLogFileReader.java

@@ -237,6 +241,10 @@ private boolean isBlockCorrupt(int blocksize) throws IOException {
      inputStream.seek(currentPos + blocksize);
    } catch (EOFException e) {
      // this is corrupt
+      // This seek is required because contract of seek() is different for naked DFSInputStream vs BufferedFSInputStream


This was one of the most tedious things to track down, phew

n3nash · 2018-04-05T06:02:06Z

hoodie-client/src/test/java/com/uber/hoodie/io/TestHoodieCommitArchiveLog.java

  private Configuration hadoopConf;
+  //NOTE : Be careful in using DFS (FileSystem.class) vs LocalFs(RawLocalFileSystem.class)
+  //The implementation and gurantees of many API's differ, for example check rename(src,dst)
+  // We need to use DFS here instead of LocalFs since the FsDataInputStream.getWrappedStream() returns a FsDataInputStream


Another extremely tedious thing to track down, wish LocalFS and DFS had same contracts :(

n3nash · 2018-04-05T06:18:42Z

@vinothchandar @bvaradar I'm keeping the BufferedInputStream changes local to the LogReader for now. We can open another issue whether we want to make this change in HoodieWrapperFileSystem sine that will require wider testing.
Also, I'm open to suggestions for the DEFAULT_BUFFER_SIZE. I don't feel like making it configurable to user since it's something more internal. I'm going to test with 8 MB in the production system tomorrow.

vinothchandar

Looks good to me. let me know how testing goes.. And yes, we can keep this localized to LogReader for now

n3nash · 2018-04-06T02:37:11Z

Results from testing in production

8 MB block size

18/04/05 21:17:34 INFO log.HoodieLogFileReader: Log HoodieLogFile {.....} has a corrupted block at 14
18/04/05 21:18:04 INFO log.HoodieLogFileReader: Next available block in HoodieLogFile {....} starts at 8240687
18/04/05 21:18:04 INFO log.HoodieLogFileReader: Log HoodieLogFile {.....} has a corrupted block at 14
18/04/05 21:18:34 INFO log.HoodieLogFileReader: Next available block in HoodieLogFile {.....} starts at 8240687

64 MB block size

18/04/05 21:41:29 INFO log.HoodieLogFileReader: Log HoodieLogFile {....} has a corrupted block at 14
18/04/05 21:41:56 INFO log.HoodieLogFileReader: Next available block in HoodieLogFile {....} starts at 8240687
18/04/05 21:41:57 INFO log.HoodieLogFileReader: Log HoodieLogFile {....} has a corrupted block at 14
18/04/05 21:42:31 INFO log.HoodieLogFileReader: Next available block in HoodieLogFile {....} starts at 8240687

256 MB block size

18/04/05 21:49:20 INFO log.HoodieLogFileReader: Log HoodieLogFile {....} has a corrupted block at 14
18/04/05 21:49:48 INFO log.HoodieLogFileReader: Next available block in HoodieLogFile {....} starts at 8240687
18/04/05 21:49:48 INFO log.HoodieLogFileReader: Log HoodieLogFile {....} has a corrupted block at 14
18/04/05 21:50:17 INFO log.HoodieLogFileReader: Next available block in HoodieLogFile {....} starts at 8240687

256 MB block size old PR

18/04/05 22:36:41 INFO log.HoodieLogFileReader: Log HoodieLogFile {....} has a corrupted block at 14
18/04/05 22:37:09 INFO log.HoodieLogFileReader: Next available block in HoodieLogFile {....} starts at 8240687
18/04/05 22:37:10 INFO log.HoodieLogFileReader: Log HoodieLogFile {....} has a corrupted block at 14
18/04/05 22:37:34 INFO log.HoodieLogFileReader: Next available block in HoodieLogFile {....} starts at 8240687

Summary for corrupt block of around 8.5MB

Block_Size	Approx Time Taken (secs)
8 MB	90
64 MB	25
256 MB	28

vinothchandar · 2018-04-06T14:28:10Z

So something like 64MB should work? its still kind of weird though 64MB seeking takes 20 sec. Is it due to some sys call overhead

n3nash · 2018-04-06T17:40:08Z

Yeah, 20 secs is way too large, I'm going to delve into what's going on soon. BTW, 64 MB works in this example since the corrupted bytes are > 8 MB and < 64 MB. At any point when the corrupted bytes are > BLOCK_SIZE, we will see a much higher amount of time taken to find the corrupt block. So, if we want to be really safe, we want to keep the BLOCK_SIZE as large as the LOG_BLOCK_SIZE. WDYT ?

n3nash · 2018-04-09T05:40:53Z

Tried a few things like use System.copy(..) instead of inputStream.readFully(..) twice but doesn't reduce the time by a lot. Essentially, each byte results in a read of (6 bytes + 4 bytes) for each MAGIC header. So, a 8.5 MB corrupt block results in 10*8000000 bytes and corresponding all operations for each byte (compare MAGIC headers) etc. To avoid large times for corrupt blocks the best buffer size is to set to BLOCK_SIZE which is 256 MB. This will add a 256MB memory overhead. The build is failing with OOM, looking into why that's happening on jenkins not on my local.

n3nash · 2018-04-15T22:59:23Z

@vinothchandar Please take a pass at this now. Updated time taken :

Block_Size	Approx Time Taken (secs)
16 MB	1
64 MB	1
256 MB	1

16 MB block

18/04/15 22:54:33 INFO log.HoodieLogFileReader: Log HoodieLogFile {....log.1} has a corrupted block at 14
18/04/15 22:54:33 INFO log.HoodieLogFileReader: Next available block in HoodieLogFile {....log.1} starts at 8240687
18/04/15 22:54:34 INFO log.HoodieLogFileReader: Log HoodieLogFile {...log.1} has a corrupted block at 14
18/04/15 22:54:35 INFO log.HoodieLogFileReader: Next available block in HoodieLogFile {..log.1} starts at 8240687

The extra time spent was in the fact that for each byte scan the code was throwing an Exception. Throwing and catching exceptions is an expensive operation. Some more info here : http://java-performance.info/throwing-an-exception-in-java-is-very-slow/. I've refactored the code to avoid throwing exceptions and instead return boolean.

vinothchandar

Just few renames

vinothchandar · 2018-04-17T21:54:13Z

hoodie-cli/src/main/java/com/uber/hoodie/cli/commands/HoodieLogFileCommand.java

@@ -172,7 +175,8 @@ public String showLogFileRecords(@CliOption(key = {
              .getTimestamp(),
          Long.valueOf(HoodieMemoryConfig.DEFAULT_MAX_MEMORY_FOR_SPILLABLE_MAP_IN_BYTES),
          Boolean.valueOf(HoodieCompactionConfig.DEFAULT_COMPACTION_LAZY_BLOCK_READ_ENABLED),
-          Boolean.valueOf(HoodieCompactionConfig.DEFAULT_COMPACTION_REVERSE_LOG_READ_ENABLED));
+          Boolean.valueOf(HoodieCompactionConfig.DEFAULT_COMPACTION_REVERSE_LOG_READ_ENABLED),
+          Integer.valueOf(HoodieMemoryConfig.DEFAULT_MAX_HDFS_STREAM_BUFFER_SIZE));


rename to : ..MAX_DFS_STREAM_BUFFER_SIZE

vinothchandar · 2018-04-17T21:54:54Z

hoodie-cli/src/main/java/com/uber/hoodie/cli/commands/HoodieLogFileCommand.java

@@ -92,6 +92,9 @@ public String showLogFileCommits(
        if (n instanceof HoodieCorruptBlock) {
          try {
            instantTime = n.getLogBlockHeader().get(HeaderMetadataType.INSTANT_TIME);
+            if (instantTime == null) {
+              throw new Exception("Invalid value " + instantTime + " for instant time ");


change to : "Invalid instant time" + null

vinothchandar · 2018-04-17T21:55:10Z

hoodie-client/src/main/java/com/uber/hoodie/config/HoodieMemoryConfig.java

@@ -45,6 +45,9 @@
  public static final String MAX_MEMORY_FOR_MERGE_PROP = "hoodie.memory.merge.max.size";
  // Property to set the max memory for compaction
  public static final String MAX_MEMORY_FOR_COMPACTION_PROP = "hoodie.memory.compaction.max.size";
+  // Property to set the max memory for hdfs inputstream buffer size
+  public static final String MAX_HDFS_STREAM_BUFFER_SIZE_PROP = "hoodie.memory.hdfs.buffer.max.size";


rename here as well

n3nash · 2018-04-17T23:39:43Z

hoodie-hadoop-mr/src/main/java/com/uber/hoodie/hadoop/realtime/HoodieRealtimeRecordReader.java

+  // Property to set the max memory for hdfs inputstream buffer size
+  public static final String MAX_HDFS_STREAM_BUFFER_SIZE_PROP = "hoodie.memory.hdfs.buffer.max.size";
+  // Setting this to lower value of 1 MB since no control over how many RecordReaders will be started in a mapper
+  public static final int DEFAULT_MAX_HDFS_STREAM_BUFFER_SIZE = 1 * 1024 * 1024; // 1 MB


@vinothchandar Kept this lower since don't know memory settings for mapper. WDYT ?

n3nash · 2018-04-17T23:40:01Z

@vinothchandar made renames and left one comment

vinothchandar · 2018-04-18T00:38:04Z

hoodie-client/src/main/java/com/uber/hoodie/config/HoodieMemoryConfig.java

@@ -45,6 +45,9 @@
  public static final String MAX_MEMORY_FOR_MERGE_PROP = "hoodie.memory.merge.max.size";
  // Property to set the max memory for compaction
  public static final String MAX_MEMORY_FOR_COMPACTION_PROP = "hoodie.memory.compaction.max.size";
+  // Property to set the max memory for hdfs inputstream buffer size
+  public static final String MAX_DFS_STREAM_BUFFER_SIZE_PROP = "hoodie.memory.hdfs.buffer.max.size";


hoodie.memory.dfs.buffer.max.size?

ahh, apologies for the oversight, will rename correctly

vinothchandar · 2018-04-18T00:38:51Z

hoodie-client/src/main/java/com/uber/hoodie/config/HoodieMemoryConfig.java

@@ -86,6 +89,12 @@ public Builder withMaxMemoryFractionPerCompaction(long maxMemoryFractionPerCompa
      return this;
    }

+    public Builder withMaxStreamBufferSize(int maxStreamBufferSize) {


rename: withMaxDFSStreamBufferSize(..)

vinothchandar · 2018-04-18T00:39:38Z

hoodie-client/src/main/java/com/uber/hoodie/config/HoodieWriteConfig.java

@@ -369,6 +369,12 @@ public Long getMaxMemoryPerCompaction() {
            props.getProperty(HoodieMemoryConfig.MAX_MEMORY_FOR_COMPACTION_PROP));
  }

+  public int getMaxBufferSize() {


make consistent with withMaxDFSStreamBufferSize above?

n3nash mentioned this pull request Apr 4, 2018

(WIP) Optimizing search for start and end of corrupt blocks when a corrupt block needs to be created #361

Closed

n3nash force-pushed the corrupt_block_optimization_v2 branch 5 times, most recently from 05fa0ae to 610402f Compare April 5, 2018 05:59

n3nash commented Apr 5, 2018

View reviewed changes

n3nash force-pushed the corrupt_block_optimization_v2 branch from 610402f to faab93d Compare April 5, 2018 06:07

n3nash changed the title ~~(WIP) Using BufferedFsInputStream to wrap FSInputStream for FSDataInputStream~~ Using BufferedFsInputStream to wrap FSInputStream for FSDataInputStream Apr 5, 2018

n3nash force-pushed the corrupt_block_optimization_v2 branch from faab93d to fee5a92 Compare April 5, 2018 06:15

vinothchandar approved these changes Apr 5, 2018

View reviewed changes

n3nash force-pushed the corrupt_block_optimization_v2 branch from fee5a92 to f3861a4 Compare April 5, 2018 21:50

n3nash force-pushed the corrupt_block_optimization_v2 branch from f3861a4 to 4b3461f Compare April 6, 2018 02:42

n3nash force-pushed the corrupt_block_optimization_v2 branch 10 times, most recently from 0ca2590 to daf9078 Compare April 11, 2018 17:18

n3nash force-pushed the corrupt_block_optimization_v2 branch 8 times, most recently from aa4ae94 to c81ee77 Compare April 15, 2018 22:41

n3nash force-pushed the corrupt_block_optimization_v2 branch 4 times, most recently from e4389ff to dd7ad31 Compare April 16, 2018 05:39

vinothchandar reviewed Apr 17, 2018

View reviewed changes

n3nash commented Apr 17, 2018

View reviewed changes

n3nash force-pushed the corrupt_block_optimization_v2 branch 3 times, most recently from cf9b23b to 5c74e61 Compare April 17, 2018 23:56

vinothchandar requested changes Apr 18, 2018

View reviewed changes

n3nash force-pushed the corrupt_block_optimization_v2 branch 2 times, most recently from 9c1a038 to 1c955fa Compare April 18, 2018 01:07

Using BufferedFsInputStream to wrap FSInputStream for FSDataInputStream

30102a4

n3nash force-pushed the corrupt_block_optimization_v2 branch from 1c955fa to 30102a4 Compare April 18, 2018 01:17

vinothchandar approved these changes Apr 18, 2018

View reviewed changes

vinothchandar merged commit c3c205f into apache:master Apr 18, 2018

vinishjail97 added a commit to vinishjail97/hudi that referenced this pull request Dec 15, 2023

Upgrade to version release-v0.24.1 (apache#373)

cc950e9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Using BufferedFsInputStream to wrap FSInputStream for FSDataInputStream #373

Using BufferedFsInputStream to wrap FSInputStream for FSDataInputStream #373

n3nash commented Apr 3, 2018

n3nash Apr 5, 2018

n3nash Apr 5, 2018

n3nash Apr 5, 2018

n3nash commented Apr 5, 2018 •

edited

vinothchandar left a comment

n3nash commented Apr 6, 2018

vinothchandar commented Apr 6, 2018

n3nash commented Apr 6, 2018 •

edited

n3nash commented Apr 9, 2018 •

edited

n3nash commented Apr 15, 2018

vinothchandar left a comment

vinothchandar Apr 17, 2018

n3nash Apr 17, 2018

vinothchandar Apr 17, 2018

n3nash Apr 17, 2018

vinothchandar Apr 17, 2018

n3nash Apr 17, 2018

n3nash Apr 17, 2018

n3nash commented Apr 17, 2018

vinothchandar Apr 18, 2018

n3nash Apr 18, 2018

vinothchandar Apr 18, 2018

n3nash Apr 18, 2018

vinothchandar Apr 18, 2018

n3nash Apr 18, 2018

Using BufferedFsInputStream to wrap FSInputStream for FSDataInputStream #373

Using BufferedFsInputStream to wrap FSInputStream for FSDataInputStream #373

Conversation

n3nash commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3nash commented Apr 5, 2018 • edited

vinothchandar left a comment

Choose a reason for hiding this comment

n3nash commented Apr 6, 2018

vinothchandar commented Apr 6, 2018

n3nash commented Apr 6, 2018 • edited

n3nash commented Apr 9, 2018 • edited

n3nash commented Apr 15, 2018

vinothchandar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3nash commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n3nash commented Apr 5, 2018 •

edited

n3nash commented Apr 6, 2018 •

edited

n3nash commented Apr 9, 2018 •

edited