> On Sept. 20, 2014, 8:09 a.m., Hyunsik Choi wrote: > > tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/ExternalSortExec.java, > > line 406 > > <https://reviews.apache.org/r/25242/diff/4/?file=693383#file693383line406> > > > > My question is the following: > > > > - Can ensure that all files whose start offset is 0 and fragment length > > is equivalent to its file length are fetched files? (because there is no > > effect if fetched files were removed) > > > > Actually, we cannot ensure the above question because some of them can > > be local files instead of fetch files if an entire range of one task result > > in previous stage is fully covered by the assigned range of this task in > > the current stage. > > > > As a result, local files can be removed in some cases. It will cause > > FileNotFound in TajoPullServer when fetch requests for removed files are > > received. > > > > Nevertheless, it will work correctly because the fact that an entire > > range of one task result in previous stage is fully covered by the assigned > > range of this task in the current stage guarantees that this task result > > file is not shared with other tasks in the current stage. It's also because > > Fetcher ignores Fetcher failure caused by FileNotFound as some workaround > > manner. But, it has potential problem because this if-condition will be > > work when Fetcher's behavior is changed. > > > > In my opinion, code readers should know the above things in order to > > fully understand this condition. Otherwise, they will have many questions. > > > > If you give some detailed comments, it would be good. Otherwise, more > > explicit condition would be good too. Even though I prefer the latter > > solution. it's up to you. If you give more comments, it would reduce the > > confusion of code readers.
I agree with you and I think that your concern is reasonable. So, I change that if-condition to remove only intermediate data generated during external sort, not input data. I also add some comments to make it clearer. - Hai Thanh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25242/#review54076 ----------------------------------------------------------- On Sept. 22, 2014, 4:32 a.m., Hai Thanh Mai wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25242/ > ----------------------------------------------------------- > > (Updated Sept. 22, 2014, 4:32 a.m.) > > > Review request for Tajo and Hyunsik Choi. > > > Repository: tajo > > > Description > ------- > > TAJO-983: Worker should directly read Intermediate data stored in localhost > rather than fetching > > Currently, worker always fetches all intermediate via Fetcher and than store > them in local file system even though some intermediate data already are > stored in local file system. It is inefficient and causes unnecessary I/O and > extra storage occupation. We should improve it. > > https://issues.apache.org/jira/browse/TAJO-983 > > > Diffs > ----- > > > tajo-core/src/main/java/org/apache/tajo/engine/planner/UniformRangePartition.java > db12285 > > tajo-core/src/main/java/org/apache/tajo/engine/planner/physical/ExternalSortExec.java > 700e34d > tajo-core/src/main/java/org/apache/tajo/worker/Fetcher.java 4867fe4 > tajo-core/src/main/java/org/apache/tajo/worker/Task.java c9c83d1 > tajo-core/src/test/java/org/apache/tajo/worker/TestFetcher.java b15d523 > tajo-storage/src/main/java/org/apache/tajo/storage/RawFile.java 1f57675 > > tajo-yarn-pullserver/src/main/java/org/apache/tajo/pullserver/PullServerAuxService.java > e6e7ce3 > > tajo-yarn-pullserver/src/main/java/org/apache/tajo/pullserver/TajoPullServerService.java > f7bc489 > > tajo-yarn-pullserver/src/main/java/org/apache/tajo/pullserver/retriever/FileChunk.java > a8b424e > > Diff: https://reviews.apache.org/r/25242/diff/ > > > Testing > ------- > > > Thanks, > > Hai Thanh Mai > >
