Re: Review request for https://issues.apache.org/jira/browse/HDFS-15413

[Bryan Beaudreault]

Thank you for the quick response here everyone!

On Fri, Nov 3, 2023 at 7:13 AM Ayush Saxena  wrote:

> I just had a quick pass, In general the logic looks good. But there is
> no test added, We should add some tests around this & cover all
> possible scenarios, EC is pretty complex & quite prone to Data
> corruption, so we should have proper test coverage around it.
>
> Xiaoqiao He has also invited folks for review, so it should get sorted soon
>
> -Ayush
>
> On Fri, 3 Nov 2023 at 08:54, Xiaoqiao He  wrote:
> >
> > Hi Bryan, Thanks for reaching out and sorry for the late response for
> this
> > JIRA issue.
> > I tried to contact one HDFS EC expert to get involved. Hope we can push
> > this forward.
> > Thanks again.
> >
> > Best Regards,
> > - He Xiaoqiao
> >
> >
> > On Fri, Nov 3, 2023 at 12:49 AM Bryan Beaudreault <
> bbeaudrea...@apache.org>
> > wrote:
> >
> > > Hello devs,
> > >
> > > There is a PR available on
> > > https://issues.apache.org/jira/browse/HDFS-15413
> > > for a few months. This problem is painful for hbase users looking to
> use
> > > Erasure Coding. Any chance a reviewer could take a look? Hopefully the
> dev
> > > who submitted the PR is still around to respond to any feedback.
> > >
> > > Thanks
> > >
>

Re: Review request for https://issues.apache.org/jira/browse/HDFS-15413

[Ayush Saxena]

I just had a quick pass, In general the logic looks good. But there is
no test added, We should add some tests around this & cover all
possible scenarios, EC is pretty complex & quite prone to Data
corruption, so we should have proper test coverage around it.

Xiaoqiao He has also invited folks for review, so it should get sorted soon

-Ayush

On Fri, 3 Nov 2023 at 08:54, Xiaoqiao He  wrote:
>
> Hi Bryan, Thanks for reaching out and sorry for the late response for this
> JIRA issue.
> I tried to contact one HDFS EC expert to get involved. Hope we can push
> this forward.
> Thanks again.
>
> Best Regards,
> - He Xiaoqiao
>
>
> On Fri, Nov 3, 2023 at 12:49 AM Bryan Beaudreault 
> wrote:
>
> > Hello devs,
> >
> > There is a PR available on
> > https://issues.apache.org/jira/browse/HDFS-15413
> > for a few months. This problem is painful for hbase users looking to use
> > Erasure Coding. Any chance a reviewer could take a look? Hopefully the dev
> > who submitted the PR is still around to respond to any feedback.
> >
> > Thanks
> >

-
To unsubscribe, e-mail: hdfs-dev-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-dev-h...@hadoop.apache.org

Re: Review request for https://issues.apache.org/jira/browse/HDFS-15413

[Xiaoqiao He]

Hi Bryan, Thanks for reaching out and sorry for the late response for this
JIRA issue.
I tried to contact one HDFS EC expert to get involved. Hope we can push
this forward.
Thanks again.

Best Regards,
- He Xiaoqiao


On Fri, Nov 3, 2023 at 12:49 AM Bryan Beaudreault 
wrote:

> Hello devs,
>
> There is a PR available on
> https://issues.apache.org/jira/browse/HDFS-15413
> for a few months. This problem is painful for hbase users looking to use
> Erasure Coding. Any chance a reviewer could take a look? Hopefully the dev
> who submitted the PR is still around to respond to any feedback.
>
> Thanks
>

Review request for https://issues.apache.org/jira/browse/HDFS-15413

[Bryan Beaudreault]

Hello devs,

There is a PR available on https://issues.apache.org/jira/browse/HDFS-15413
for a few months. This problem is painful for hbase users looking to use
Erasure Coding. Any chance a reviewer could take a look? Hopefully the dev
who submitted the PR is still around to respond to any feedback.

Thanks

Review Request

[Brahma Reddy Battula]

Hi All

If you get chance, can you please review the following. I feel, these two are 
good candidates for 2.8.2 which is going to release soon.

https://issues.apache.org/jira/browse/HDFS-12084

https://issues.apache.org/jira/browse/HDFS-12299



--Brahma Reddy Battula

Review request for HDFS-8631

[surendrasingh lilhore]

Hi All,

Can someone review HDFS-8631. This jira implement the quota API in WebHdfs
File System. It will be help full for managing hdfs quota from REST API.

Thanks
-Surendra

[Review Request] For HDFS patches

[Vinayakumar B]

Hi all,

I have posted patches on following Jiras and all are in Patch available state. 
Some of them are there in that state for quite some time.

Please review these patches and post your comments.  Thanks in advance.

HDFS-5112
HDFS-5031
HDFS-5014
HDFS-4516
HDFS-4223
HDFS-3618
HDFS-3493
HDFS-3405

Regards,
   Vinayakumar B.,


This e-mail and attachments contain confidential information from HUAWEI,
which is intended only for the person or entity whose address is listed
above. Any use of the information contained herein in any way (including,
but not limited to, total or partial disclosure, reproduction, or
dissemination) by persons other than the intended recipient's) is
prohibited. If you receive this e-mail in error, please notify the sender by
phone or email immediately and delete it!

Code Review Request

[Wang Tianhong]

Please review patch:
https://issues.apache.org/jira/browse/HDFS-4730
https://issues.apache.org/jira/browse/HDFS-4709
https://issues.apache.org/jira/browse/HDFS-4681

Thanks

Re: Review Request: HDFS-3058 HA: Bring BookKeeperJournalManager up to date with HA changes

[Ivan Kelly]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4230/
---

(Updated 2012-05-18 15:43:52.712478)


Review request for hadoop-hdfs.


Summary
---

There's a couple of TODO(HA) comments in the BookKeeperJournalManager code. 
This JIRA is to address those.


This addresses bug HDFS-3058.
http://issues.apache.org/jira/browse/HDFS-3058


Diffs (updated)
-

  hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/pom.xml 380ef62 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java
 9d070d9 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperJournalManager.java
 047efd5 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/BKJMUtil.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/TestBookKeeperAsHASharedDir.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/TestBookKeeperHACheckpoints.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/TestBookKeeperJournalManager.java
 5937fa8 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogTestUtil.java
 41f0292 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
 c144906 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/MiniDFSCluster.java
 b418fcf 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestStandbyCheckpoints.java
 5440c38 

Diff: https://reviews.apache.org/r/4230/diff


Testing
---


Thanks,

Ivan

Re: Review Request: HDFS-3058 HA: Bring BookKeeperJournalManager up to date with HA changes

[Ivan Kelly]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4230/
---

(Updated 2012-05-10 14:44:08.706764)


Review request for hadoop-hdfs.


Summary
---

There's a couple of TODO(HA) comments in the BookKeeperJournalManager code. 
This JIRA is to address those.


This addresses bug HDFS-3058.
http://issues.apache.org/jira/browse/HDFS-3058


Diffs (updated)
-

  hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/pom.xml 380ef62 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java
 9d070d9 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperJournalManager.java
 7fa9026 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/BKJMUtil.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/TestBookKeeperAsHASharedDir.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/TestBookKeeperHACheckpoints.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/TestBookKeeperJournalManager.java
 b949bc2 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogTestUtil.java
 41f0292 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
 c144906 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/MiniDFSCluster.java
 3810614 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestStandbyCheckpoints.java
 5440c38 

Diff: https://reviews.apache.org/r/4230/diff


Testing
---


Thanks,

Ivan

Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)

[Henry Robinson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/
---

(Updated 2012-03-20 16:29:56.616292)


Review request for hadoop-hdfs and Todd Lipcon.


Changes
---

Review responses


Summary
---

New patch for HDFS-2834 (I can't update the old review request).


This addresses bug HDFS-2834.
http://issues.apache.org/jira/browse/HDFS-2834


Diffs (updated)
-

  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java
 dfab730 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
 cc61697 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
 4187f1c 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
 71c8a50 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java
 b7da8d4 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
 ea24777 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java
 9d4f4a2 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java
 bbd0012 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
 f4052bb 

Diff: https://reviews.apache.org/r/4212/diff


Testing
---


Thanks,

Henry

Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)

[Henry Robinson]

> On 2012-03-20 01:27:50, Todd Lipcon wrote:
> > hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java,
> >  line 44
> > <https://reviews.apache.org/r/4212/diff/2/?file=90213#file90213line44>
> >
> > shouldn't this be true?

Oops, yes. Thankfully the test still passes when it's testing the right path...


> On 2012-03-20 01:27:50, Todd Lipcon wrote:
> > hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java,
> >  lines 81-82
> > <https://reviews.apache.org/r/4212/diff/2/?file=90213#file90213line81>
> >
> > no reason to use DFSClient here. Instead you can just use the 
> > filesystem, right? Then downcast the stream you get back?

Good point - no need even to downcast since FSDataInputStream has the API.


> On 2012-03-20 01:27:50, Todd Lipcon wrote:
> > hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java,
> >  line 104
> > <https://reviews.apache.org/r/4212/diff/2/?file=90213#file90213line104>
> >
> > don't you want an assert on sawException here? You can also use 
> > GenericTestUtils.assertExceptionContains() if you want to check the text of 
> > it

Good catch. No particular need to assert the content of the exception - any 
checksum error is good enough here. 


> On 2012-03-20 01:27:50, Todd Lipcon wrote:
> > hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java,
> >  lines 562-564
> > <https://reviews.apache.org/r/4212/diff/2/?file=90207#file90207line562>
> >
> > this comment seems like it's in the wrong spot, since the code that 
> > comes after it doesn't reference offsetFromChunkBoundary.

I removed the comment, it's covered by the comment at line 549.


- Henry


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/#review6103
---


On 2012-03-09 00:47:24, Henry Robinson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4212/
> -------
> 
> (Updated 2012-03-09 00:47:24)
> 
> 
> Review request for hadoop-hdfs and Todd Lipcon.
> 
> 
> Summary
> ---
> 
> New patch for HDFS-2834 (I can't update the old review request).
> 
> 
> This addresses bug HDFS-2834.
> http://issues.apache.org/jira/browse/HDFS-2834
> 
> 
> Diffs
> -
> 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java
>  dfab730 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
>  cc61697 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
>  4187f1c 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
>  2b817ff 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java
>  b7da8d4 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
>  ea24777 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java
>  9d4f4a2 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
>  PRE-CREATION 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java
>  bbd0012 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
>  eb2a1d8 
> 
> Diff: https://reviews.apache.org/r/4212/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Henry
> 
>

Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/#review6103
---


Real close now!


hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment13128>

this comment seems like it's in the wrong spot, since the code that comes 
after it doesn't reference offsetFromChunkBoundary.



hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment13130>

shouldn't this be true?



hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment13132>

no reason to use DFSClient here. Instead you can just use the filesystem, 
right? Then downcast the stream you get back?



hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment13131>

don't you want an assert on sawException here? You can also use 
GenericTestUtils.assertExceptionContains() if you want to check the text of it


- Todd


On 2012-03-09 00:47:24, Henry Robinson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4212/
> -------
> 
> (Updated 2012-03-09 00:47:24)
> 
> 
> Review request for hadoop-hdfs and Todd Lipcon.
> 
> 
> Summary
> ---
> 
> New patch for HDFS-2834 (I can't update the old review request).
> 
> 
> This addresses bug HDFS-2834.
> http://issues.apache.org/jira/browse/HDFS-2834
> 
> 
> Diffs
> -
> 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java
>  dfab730 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
>  cc61697 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
>  4187f1c 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
>  2b817ff 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java
>  b7da8d4 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
>  ea24777 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java
>  9d4f4a2 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
>  PRE-CREATION 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java
>  bbd0012 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
>  eb2a1d8 
> 
> Diff: https://reviews.apache.org/r/4212/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Henry
> 
>

Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)

[Henry Robinson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/
---

(Updated 2012-03-09 00:47:24.765130)


Review request for hadoop-hdfs and Todd Lipcon.


Summary
---

New patch for HDFS-2834 (I can't update the old review request).


This addresses bug HDFS-2834.
http://issues.apache.org/jira/browse/HDFS-2834


Diffs
-

  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java
 dfab730 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
 cc61697 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
 4187f1c 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
 2b817ff 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java
 b7da8d4 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
 ea24777 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java
 9d4f4a2 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java
 bbd0012 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
 eb2a1d8 

Diff: https://reviews.apache.org/r/4212/diff


Testing
---


Thanks,

Henry

Review Request: HDFS-3058 HA: Bring BookKeeperJournalManager up to date with HA changes

[Ivan Kelly]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4230/
---

Review request for hadoop-hdfs.


Summary
---

There's a couple of TODO(HA) comments in the BookKeeperJournalManager code. 
This JIRA is to address those.


This addresses bug HDFS-3058.
http://issues.apache.org/jira/browse/HDFS-3058


Diffs
-

  hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/pom.xml 1fba846 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperEditLogInputStream.java
 636471a 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/main/java/org/apache/hadoop/contrib/bkjournal/BookKeeperJournalManager.java
 047efd5 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/BKJMUtil.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/TestBookKeeperAsHASharedDir.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/TestBookKeeperHACheckpoints.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/contrib/bkjournal/TestBookKeeperJournalManager.java
 5937fa8 
  
hadoop-hdfs-project/hadoop-hdfs/src/contrib/bkjournal/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogTestUtil.java
 6557b96 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
 7c630d7 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/MiniDFSCluster.java
 bec 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/ha/TestStandbyCheckpoints.java
 5440c38 

Diff: https://reviews.apache.org/r/4230/diff


Testing
---


Thanks,

Ivan

Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/#review5665
---


some stuff around error cases here -- I think you'd run into these bugs if you 
hit a checksum error, since the "retry on different node" path would trigger in 
DFSInputStream, but the target buffer's position would be prematurely updated 
from prior attempts.

But getting close!

Is it possible to split out the test refactor change from this one? Hard to 
tell what's changed in the tests vs just refactored. If it's a pain in the butt 
I'll look more closely.


hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java
<https://reviews.apache.org/r/4212/#comment12328>

unused import?



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12329>

please use javadoc style comments for these, rather than //s



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12330>

I envision some silly user setting the buffer size < bytesPerChecksum and 
getting screwed here. Worth a check for valid range here (throwing IOE or 
IllegalArgumentException or something)



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12331>

This isn't your bug, but I just realized there's a potential leak here if 
the skip() calls below failed. I think we need to add a try..finally{} which 
returns these buffers to the pool if construction fails



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12332>

should this line go in a finally{} clause? Otherwise if 'to' doesn't have 
enough space for the copy, we'd end up leaving 'from' with the modified limit



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12333>

in finally{} clause perhaps, so the limit isn't messed up by a failed read



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12337>

the error conditions here don't quite work right.
For example, if a checksum error occurs, the buffer's position will still 
be updated.

Above, there's a similar problem if "phase 1" succeeds but "phase 2" 
encounters an error -- the position will change but the method will throw.



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12338>

no '-'s needed in javadoc



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
<https://reviews.apache.org/r/4212/#comment12336>

do you really need synchronization here and below? it seems like the 
strategy is being called from a synchronized context, so this is redundant. 
Removing this also allows the inner classes to be static which I think is 
preferable



hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
<https://reviews.apache.org/r/4212/#comment12342>

worth printing the exception message



hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
<https://reviews.apache.org/r/4212/#comment12340>

do you need to do something here to clear the JVM exception state? you'll 
probably have an OOME pending here. Probably good to use errNoFromException so 
that you get a reasonable error code (assume it does something good with OOME).



hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
<https://reviews.apache.org/r/4212/#comment12343>

is there any logging setting for libhdfs? seems like you'd want to print 
the stack trace if some debug flag is set



hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
<https://reviews.apache.org/r/4212/#comment12344>

style, } else {


- Todd


On 2012-03-06 23:14:07, Henry Robinson wrote:
> 
> -----------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4212/
> -------
> 
> (Updated 2012-03-06 23:14:07)
> 
> 
> Review request for hadoop-hdfs and Todd Lipcon.
> 
> 
> Summary
> ---
> 
> New patch for HDFS-2834 (I can't update the old review request).
> 
> 
> This addresses bug HDFS-2834.
> http://issues.apache.org/jira/browse/HDFS-2834
> 
> 
>

Review Request: ByteBuffer-based read API for DFSInputStream (review 2)

[Henry Robinson]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/
---

Review request for hadoop-hdfs and Todd Lipcon.


Summary
---

New patch for HDFS-2834 (I can't update the old review request).


This addresses bug HDFS-2834.
http://issues.apache.org/jira/browse/HDFS-2834


Diffs
-

  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java
 dfab730 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
 cc61697 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
 4187f1c 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
 2b817ff 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java
 b7da8d4 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
 ea24777 
  hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h 0ee29d5 
  hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c 7371caf 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java
 9d4f4a2 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelLocalRead.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java
 bbd0012 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelReadUtil.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
 eb2a1d8 

Diff: https://reviews.apache.org/r/4212/diff


Testing
---


Thanks,

Henry

Re: Review Request: ByteBuffer-based read API for DFSInputStream

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4184/#review5622
---

Ship it!


first pass looks good. A few questions:
- do you have before/after benchmarks for the old TestParallelRead with this 
patch? ie make sure this patch doesn't regress performance of the "normal" 
array-based read path?
- have you run the various randomized tests overnight or so to get good 
coverage of all the cases?

I didn't look carefully at the libhdfs code, since you said offline there's 
another rev of that coming


hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4184/#comment12234>

can you add javadoc explaining this var (even though it's not new)?



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4184/#comment12233>

I'd move this inside the if statement below. Then in the second "phase", 
re-declare it under that scope, since the "len" variable doesn't ever carry 
over between phases (and in fact becomes invalid)



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4184/#comment12232>

Seems like this area could be optimized a bit to avoid the object creation 
here...

In one case, slowReadBuff.remaining() <= len, in which case you can 
directly do buf.put(slowReadBuff).

In the other case, you could probably save off the limit of slowReadBuff, 
lower the limit to len, do the read, then set the limit again.



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4184/#comment12235>

this code's very similar to line 358-362. Can we extract a utility function 
which we could optimize as I mentioned above to avoid object allocation?



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4184/#comment12236>

I think it might simplify things if you used this function only for the 
case where verifyChecksum == true. Then above, the non-checksummed read case 
would just call fillBuffer directly. What do you think?

My reasoning is that this function is called 'readChunksIntoByteBuffer' 
implying that it always reads a multiple of checksum chunk size, which isn't 
really a requirement when not verifying checksums



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4184/#comment12237>

maybe TRACE is more appropriate here. Also LOG.trace() inside



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
<https://reviews.apache.org/r/4184/#comment12238>

remove commented out code



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
<https://reviews.apache.org/r/4184/#comment12239>

I think it's worth making these actual static inner classes with names 
instead of anonymous, for prettier stack traces.

I remember we discussed a few weeks back whether the creation of these 
lambdas would harm performance for short reads. Do you have any results for 
TestParallelRead performance for the lambda-based approach here vs the uglier 
approach



hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
<https://reviews.apache.org/r/4184/#comment12240>

goofy empty clause. Other goofy indentation here.


- Todd


On 2012-03-05 21:00:55, Todd Lipcon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4184/
> ---
> 
> (Updated 2012-03-05 21:00:55)
> 
> 
> Review request for hadoop-hdfs and Todd Lipcon.
> 
> 
> Summary
> ---
> 
> Posting patch to RB on behalf of Henry.
> 
> 
> This addresses bug HDFS-2834.
> http://issues.apache.org/jira/browse/HDFS-2834
> 
> 
> Diffs
> -
> 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java
>  dfab730 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
>  cc61697 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
>  2b817ff 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java
>  b7da8d4 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java

Review Request: ByteBuffer-based read API for DFSInputStream

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4184/
---

Review request for hadoop-hdfs and Todd Lipcon.


Summary
---

Posting patch to RB on behalf of Henry.


This addresses bug HDFS-2834.
http://issues.apache.org/jira/browse/HDFS-2834


Diffs
-

  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java
 dfab730 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
 cc61697 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
 2b817ff 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java
 b7da8d4 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java
 ea24777 
  hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h 0ee29d5 
  hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c 7371caf 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java
 9d4f4a2 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelLocalRead.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java
 bbd0012 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelReadUtil.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java
 eb2a1d8 

Diff: https://reviews.apache.org/r/4184/diff


Testing
---


Thanks,

Todd

Re: Review request: trunk->HDFS-1623 merge

[Todd Lipcon]

Thanks Suresh. I committed the merge to the branch.

-Todd

On Thu, Feb 9, 2012 at 4:32 PM, Suresh Srinivas  wrote:
> I looked at the merge. It looks good. +1.
>
> On Wed, Feb 8, 2012 at 9:08 PM, Todd Lipcon  wrote:
>
>> The branch developed some new conflicts due to recent changes in trunk
>> affecting the RPC between the DN and the NN (the "StorageReport"
>> stuff). I've done a new merge to address these conflicts here:
>>
>> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120208
>>
>> I've also addressed Aaron's comments in the thread above.
>> I ran the unit tests on the branch and they passed.
>>
>> Thanks
>> -Todd
>>
>> On Fri, Feb 3, 2012 at 4:44 PM, Aaron T. Myers  wrote:
>> > Hey Todd,
>> >
>> > The merge largely looks good. I agree with the general approach you
>> took. A
>> > few small comments:
>> >
>> > 1. There's a comment in the OP_ADD case blockabout handling OP_CLOSE.
>> This
>> > makes sense in 0.22/0.23/0.24, but in the HA branch the OP_ADD and
>> OP_CLOSE
>> > cases are completely separate case blocks. I actually find this whole
>> > comment a little confusing, since it numbers the cases we have to handle,
>> > but those numbers aren't referenced anywhere else.
>> >
>> > 2. You mentioned in your message that you don't handle the (invalid) case
>> > of OP_ADD on a new file containing updated blocks, but it looks like the
>> > code actually does, though the code also mentions that we should add a
>> > sanity check that this is actually can't occur. Seems like we should
>> clean
>> > up this inconsistency. I agree that adding asserting this case doesn't
>> > occur is the right way to go.
>> >
>> > 3. If we go with my suggestion in (2), we can also move the call to
>> > FSEditLogLoader#updateBlocks to only the case of OP_ADD for an existing
>> > file, and then get rid of the "INodeFile newFile = oldFile" assignment,
>> > which I found kind of confusing at first. (Though I do see why it's
>> correct
>> > as-implemented.) If you don't go with my suggestion in (2), please add a
>> > comment explaining the assignment.
>> >
>> > Otherwise looks good. Merge away.
>> >
>> > --
>> > Aaron T. Myers
>> > Software Engineer, Cloudera
>> >
>> >
>> >
>> > On Fri, Feb 3, 2012 at 2:10 PM, Todd Lipcon  wrote:
>> >
>> >> I've got a merge pending of trunk into HDFS-1623 -- it was a bit
>> >> complicated so wanted to ask for another set of eyes:
>> >> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120203
>> >> (using github since it's hard to review a merge patch via JIRA)
>> >>
>> >> The interesting bit of the merge was to deal with conflicts with
>> >> HDFS-2718. To summarize the changes I had to make:
>> >> - in the HDFS-1623 branch, we don't deal with the case where OP_ADD
>> >> contains blocks on a new file -- this is a case that doesn't happen on
>> >> real clusters, but currently happens with synthetic logs generated
>> >> from the CreateEditLogs tool. I added a TODO to add a sanity check
>> >> here and will address as a follow-up. Given the difference between
>> >> trunk and branch, there were a couple of small changes that propagated
>> >> into unprotectedAddFile
>> >> - In the HDFS-1623 branch we had already implemented the
>> >> "updateBlocks" call inside FSEditLogLoader. I used that existing
>> >> implementation rather than adding the new one in FSDirectory, since
>> >> this function had some other changes related to HA in the branch
>> >> version.
>> >>
>> >> I'll wait for a +1 before committing. I ran all of the unit tests and
>> >> they passed.
>> >>
>> >> -Todd
>> >> --
>> >> Todd Lipcon
>> >> Software Engineer, Cloudera
>> >>
>>
>>
>>
>> --
>> Todd Lipcon
>> Software Engineer, Cloudera
>>



-- 
Todd Lipcon
Software Engineer, Cloudera

Re: Review request: trunk->HDFS-1623 merge

[Suresh Srinivas]

I looked at the merge. It looks good. +1.

On Wed, Feb 8, 2012 at 9:08 PM, Todd Lipcon  wrote:

> The branch developed some new conflicts due to recent changes in trunk
> affecting the RPC between the DN and the NN (the "StorageReport"
> stuff). I've done a new merge to address these conflicts here:
>
> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120208
>
> I've also addressed Aaron's comments in the thread above.
> I ran the unit tests on the branch and they passed.
>
> Thanks
> -Todd
>
> On Fri, Feb 3, 2012 at 4:44 PM, Aaron T. Myers  wrote:
> > Hey Todd,
> >
> > The merge largely looks good. I agree with the general approach you
> took. A
> > few small comments:
> >
> > 1. There's a comment in the OP_ADD case blockabout handling OP_CLOSE.
> This
> > makes sense in 0.22/0.23/0.24, but in the HA branch the OP_ADD and
> OP_CLOSE
> > cases are completely separate case blocks. I actually find this whole
> > comment a little confusing, since it numbers the cases we have to handle,
> > but those numbers aren't referenced anywhere else.
> >
> > 2. You mentioned in your message that you don't handle the (invalid) case
> > of OP_ADD on a new file containing updated blocks, but it looks like the
> > code actually does, though the code also mentions that we should add a
> > sanity check that this is actually can't occur. Seems like we should
> clean
> > up this inconsistency. I agree that adding asserting this case doesn't
> > occur is the right way to go.
> >
> > 3. If we go with my suggestion in (2), we can also move the call to
> > FSEditLogLoader#updateBlocks to only the case of OP_ADD for an existing
> > file, and then get rid of the "INodeFile newFile = oldFile" assignment,
> > which I found kind of confusing at first. (Though I do see why it's
> correct
> > as-implemented.) If you don't go with my suggestion in (2), please add a
> > comment explaining the assignment.
> >
> > Otherwise looks good. Merge away.
> >
> > --
> > Aaron T. Myers
> > Software Engineer, Cloudera
> >
> >
> >
> > On Fri, Feb 3, 2012 at 2:10 PM, Todd Lipcon  wrote:
> >
> >> I've got a merge pending of trunk into HDFS-1623 -- it was a bit
> >> complicated so wanted to ask for another set of eyes:
> >> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120203
> >> (using github since it's hard to review a merge patch via JIRA)
> >>
> >> The interesting bit of the merge was to deal with conflicts with
> >> HDFS-2718. To summarize the changes I had to make:
> >> - in the HDFS-1623 branch, we don't deal with the case where OP_ADD
> >> contains blocks on a new file -- this is a case that doesn't happen on
> >> real clusters, but currently happens with synthetic logs generated
> >> from the CreateEditLogs tool. I added a TODO to add a sanity check
> >> here and will address as a follow-up. Given the difference between
> >> trunk and branch, there were a couple of small changes that propagated
> >> into unprotectedAddFile
> >> - In the HDFS-1623 branch we had already implemented the
> >> "updateBlocks" call inside FSEditLogLoader. I used that existing
> >> implementation rather than adding the new one in FSDirectory, since
> >> this function had some other changes related to HA in the branch
> >> version.
> >>
> >> I'll wait for a +1 before committing. I ran all of the unit tests and
> >> they passed.
> >>
> >> -Todd
> >> --
> >> Todd Lipcon
> >> Software Engineer, Cloudera
> >>
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>

Re: Review request: trunk->HDFS-1623 merge

[Todd Lipcon]

Updated the merge against current trunk and HA branch. Here's the github link:
https://github.com/toddlipcon/hadoop-common/commits/ha-merge-20120209

And the relevant diff reproduced below.

If this looks mostly good, please +1 - we can continue to make
improvements on the branch, but redoing the merges as both branches
move underneath takes a lot of time.

-Todd

diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdf
index 96840f6..bf1ec99 100644
--- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/serve
+++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/serve
@@ -197,9 +197,12 @@ public class FSEditLogLoader {
   permissions = addCloseOp.permissions;
 }
 long blockSize = addCloseOp.blockSize;
-
-// TODO: we should add a sanity check that there are no blocks
-// in this op, and simplify the code below!
+
+// Versions of HDFS prior to 0.17 may log an OP_ADD transaction
+// which includes blocks in it. When we update the minimum
+// upgrade version to something more recent than 0.17, we can
+// simplify this code by asserting that OP_ADD transactions
+// don't have any blocks.

 // Older versions of HDFS does not store the block size in inode.
 // If the file has more than one block, use the size of the
@@ -237,13 +240,9 @@ public class FSEditLogLoader {
 }
   }
   // Fall-through for case 2.
-  // TODO: this is below the if/else above in order to handle the
-  // case where OP_ADD is used to add a new file, and the new
-  // file has blocks in the first opcode. However, this case
-  // doesn't happen in practice. Will address in a separate
-  // JIRA so as to avoid changing too much code in a merge commit.
+  // Regardless of whether it's a new file or an updated file,
+  // update the block list.
   updateBlocks(fsDir, addCloseOp, newFile);
-
   break;
 }
 case OP_CLOSE: {


On Thu, Feb 9, 2012 at 1:20 PM, Todd Lipcon  wrote:
> On Thu, Feb 9, 2012 at 12:54 PM, Konstantin Shvachko
>  wrote:
>> Ok I misunderstood the current direction of the merge.
>>
>> On the review request:
>>
>>> we don't deal with the case where OP_ADD
>>> contains blocks on a new file -- this is a case that doesn't happen on
>>> real clusters, but currently happens with synthetic logs generated
>>> from the CreateEditLogs tool.
>>
>> I intentionally did not remove handling of new files with blocks (non
>> empty). The reason is potential issues with backward compatibility. If
>> any HDFS version in the past produced such transactions the new
>> version must be able to read them. I thought it is easier to retain
>> the support for this type of transactions rather than checking all
>> past versions.
>> I would not recommend restricting OP_ADD in the way that it requires
>> new files to be empty.
>
> Good point. I checked back in old versions and it appears that we had
> this behavior in 0.16 and below (removed in HADOOP-2345) in 0.17.
>
> I'll update the merge to continue to support this old behavior, and
> leave a note that it could be simplified by bumping our minimum
> upgrade version to 0.17 or 0.18 (which seems entirely reasonable given
> they're ~4 years old).
>
> Will report back when a new patch is up for review.
>
> -Todd
>
>>
>> Thanks,
>> --Konstantin
>>
>> On Thu, Feb 9, 2012 at 10:57 AM, Todd Lipcon  wrote:
>>> Hi Konstantin,
>>>
>>> To be clear, this review request is a merge from the trunk branch into
>>> the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
>>> much daily since the project began, so that we track trunk closely.
>>> The idea is so that we don't have unexpected integration issues when
>>> we do the merge the _other_ way when it's ready.
>>>
>>> When we propose the merge *into* trunk we'll definitely address your
>>> questions below. We are indeed already running cluster tests at
>>> 100-node scale with failovers and both MR and HBase workloads, though
>>> have not done a formal performance comparison at this point.
>>>
>>> -Todd
>>>
>>> On Thu, Feb 9, 2012 at 10:54 AM, Konstantin Shvachko
>>>  wrote:
>>>> I was wondering
>>>> 1. What was the test plan that has been executed for testing this
>>>> implementation of HA? Besides unit tests.
>>>> 2. Have you done any benchmarks, comparing current cluster performance
>>>> against the branch. Would be good to have numbers for both cases with
>>>> HA off and 

Re: Review request: trunk->HDFS-1623 merge

[Todd Lipcon]

On Thu, Feb 9, 2012 at 12:54 PM, Konstantin Shvachko
 wrote:
> Ok I misunderstood the current direction of the merge.
>
> On the review request:
>
>> we don't deal with the case where OP_ADD
>> contains blocks on a new file -- this is a case that doesn't happen on
>> real clusters, but currently happens with synthetic logs generated
>> from the CreateEditLogs tool.
>
> I intentionally did not remove handling of new files with blocks (non
> empty). The reason is potential issues with backward compatibility. If
> any HDFS version in the past produced such transactions the new
> version must be able to read them. I thought it is easier to retain
> the support for this type of transactions rather than checking all
> past versions.
> I would not recommend restricting OP_ADD in the way that it requires
> new files to be empty.

Good point. I checked back in old versions and it appears that we had
this behavior in 0.16 and below (removed in HADOOP-2345) in 0.17.

I'll update the merge to continue to support this old behavior, and
leave a note that it could be simplified by bumping our minimum
upgrade version to 0.17 or 0.18 (which seems entirely reasonable given
they're ~4 years old).

Will report back when a new patch is up for review.

-Todd

>
> Thanks,
> --Konstantin
>
> On Thu, Feb 9, 2012 at 10:57 AM, Todd Lipcon  wrote:
>> Hi Konstantin,
>>
>> To be clear, this review request is a merge from the trunk branch into
>> the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
>> much daily since the project began, so that we track trunk closely.
>> The idea is so that we don't have unexpected integration issues when
>> we do the merge the _other_ way when it's ready.
>>
>> When we propose the merge *into* trunk we'll definitely address your
>> questions below. We are indeed already running cluster tests at
>> 100-node scale with failovers and both MR and HBase workloads, though
>> have not done a formal performance comparison at this point.
>>
>> -Todd
>>
>> On Thu, Feb 9, 2012 at 10:54 AM, Konstantin Shvachko
>>  wrote:
>>> I was wondering
>>> 1. What was the test plan that has been executed for testing this
>>> implementation of HA? Besides unit tests.
>>> 2. Have you done any benchmarks, comparing current cluster performance
>>> against the branch. Would be good to have numbers for both cases with
>>> HA off and HA on.
>>>
>>> I'll post these questions to the jira as well.
>>>
>>> Thanks,
>>> --Konstantin
>>>
>>> On Wed, Feb 8, 2012 at 9:08 PM, Todd Lipcon  wrote:
>>>> The branch developed some new conflicts due to recent changes in trunk
>>>> affecting the RPC between the DN and the NN (the "StorageReport"
>>>> stuff). I've done a new merge to address these conflicts here:
>>>>
>>>> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120208
>>>>
>>>> I've also addressed Aaron's comments in the thread above.
>>>> I ran the unit tests on the branch and they passed.
>>>>
>>>> Thanks
>>>> -Todd
>>>>
>>>> On Fri, Feb 3, 2012 at 4:44 PM, Aaron T. Myers  wrote:
>>>>> Hey Todd,
>>>>>
>>>>> The merge largely looks good. I agree with the general approach you took. 
>>>>> A
>>>>> few small comments:
>>>>>
>>>>> 1. There's a comment in the OP_ADD case blockabout handling OP_CLOSE. This
>>>>> makes sense in 0.22/0.23/0.24, but in the HA branch the OP_ADD and 
>>>>> OP_CLOSE
>>>>> cases are completely separate case blocks. I actually find this whole
>>>>> comment a little confusing, since it numbers the cases we have to handle,
>>>>> but those numbers aren't referenced anywhere else.
>>>>>
>>>>> 2. You mentioned in your message that you don't handle the (invalid) case
>>>>> of OP_ADD on a new file containing updated blocks, but it looks like the
>>>>> code actually does, though the code also mentions that we should add a
>>>>> sanity check that this is actually can't occur. Seems like we should clean
>>>>> up this inconsistency. I agree that adding asserting this case doesn't
>>>>> occur is the right way to go.
>>>>>
>>>>> 3. If we go with my suggestion in (2), we can also move the call to
>>>>> FSEditLogLoader#updateBlocks to onl

Re: Review request: trunk->HDFS-1623 merge

[Konstantin Shvachko]

Ok I misunderstood the current direction of the merge.

On the review request:

> we don't deal with the case where OP_ADD
> contains blocks on a new file -- this is a case that doesn't happen on
> real clusters, but currently happens with synthetic logs generated
> from the CreateEditLogs tool.

I intentionally did not remove handling of new files with blocks (non
empty). The reason is potential issues with backward compatibility. If
any HDFS version in the past produced such transactions the new
version must be able to read them. I thought it is easier to retain
the support for this type of transactions rather than checking all
past versions.
I would not recommend restricting OP_ADD in the way that it requires
new files to be empty.

Thanks,
--Konstantin

On Thu, Feb 9, 2012 at 10:57 AM, Todd Lipcon  wrote:
> Hi Konstantin,
>
> To be clear, this review request is a merge from the trunk branch into
> the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
> much daily since the project began, so that we track trunk closely.
> The idea is so that we don't have unexpected integration issues when
> we do the merge the _other_ way when it's ready.
>
> When we propose the merge *into* trunk we'll definitely address your
> questions below. We are indeed already running cluster tests at
> 100-node scale with failovers and both MR and HBase workloads, though
> have not done a formal performance comparison at this point.
>
> -Todd
>
> On Thu, Feb 9, 2012 at 10:54 AM, Konstantin Shvachko
>  wrote:
>> I was wondering
>> 1. What was the test plan that has been executed for testing this
>> implementation of HA? Besides unit tests.
>> 2. Have you done any benchmarks, comparing current cluster performance
>> against the branch. Would be good to have numbers for both cases with
>> HA off and HA on.
>>
>> I'll post these questions to the jira as well.
>>
>> Thanks,
>> --Konstantin
>>
>> On Wed, Feb 8, 2012 at 9:08 PM, Todd Lipcon  wrote:
>>> The branch developed some new conflicts due to recent changes in trunk
>>> affecting the RPC between the DN and the NN (the "StorageReport"
>>> stuff). I've done a new merge to address these conflicts here:
>>>
>>> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120208
>>>
>>> I've also addressed Aaron's comments in the thread above.
>>> I ran the unit tests on the branch and they passed.
>>>
>>> Thanks
>>> -Todd
>>>
>>> On Fri, Feb 3, 2012 at 4:44 PM, Aaron T. Myers  wrote:
>>>> Hey Todd,
>>>>
>>>> The merge largely looks good. I agree with the general approach you took. A
>>>> few small comments:
>>>>
>>>> 1. There's a comment in the OP_ADD case blockabout handling OP_CLOSE. This
>>>> makes sense in 0.22/0.23/0.24, but in the HA branch the OP_ADD and OP_CLOSE
>>>> cases are completely separate case blocks. I actually find this whole
>>>> comment a little confusing, since it numbers the cases we have to handle,
>>>> but those numbers aren't referenced anywhere else.
>>>>
>>>> 2. You mentioned in your message that you don't handle the (invalid) case
>>>> of OP_ADD on a new file containing updated blocks, but it looks like the
>>>> code actually does, though the code also mentions that we should add a
>>>> sanity check that this is actually can't occur. Seems like we should clean
>>>> up this inconsistency. I agree that adding asserting this case doesn't
>>>> occur is the right way to go.
>>>>
>>>> 3. If we go with my suggestion in (2), we can also move the call to
>>>> FSEditLogLoader#updateBlocks to only the case of OP_ADD for an existing
>>>> file, and then get rid of the "INodeFile newFile = oldFile" assignment,
>>>> which I found kind of confusing at first. (Though I do see why it's correct
>>>> as-implemented.) If you don't go with my suggestion in (2), please add a
>>>> comment explaining the assignment.
>>>>
>>>> Otherwise looks good. Merge away.
>>>>
>>>> --
>>>> Aaron T. Myers
>>>> Software Engineer, Cloudera
>>>>
>>>>
>>>>
>>>> On Fri, Feb 3, 2012 at 2:10 PM, Todd Lipcon  wrote:
>>>>
>>>>> I've got a merge pending of trunk into HDFS-1623 -- it was a bit
>>>>> complicated so wanted to ask for another set of eyes:
>>>>> https://gi

Re: Review request: trunk->HDFS-1623 merge

[Todd Lipcon]

Hi Konstantin,

To be clear, this review request is a merge from the trunk branch into
the HA branch (NOT a merge INTO trunk) - we've been doing these pretty
much daily since the project began, so that we track trunk closely.
The idea is so that we don't have unexpected integration issues when
we do the merge the _other_ way when it's ready.

When we propose the merge *into* trunk we'll definitely address your
questions below. We are indeed already running cluster tests at
100-node scale with failovers and both MR and HBase workloads, though
have not done a formal performance comparison at this point.

-Todd

On Thu, Feb 9, 2012 at 10:54 AM, Konstantin Shvachko
 wrote:
> I was wondering
> 1. What was the test plan that has been executed for testing this
> implementation of HA? Besides unit tests.
> 2. Have you done any benchmarks, comparing current cluster performance
> against the branch. Would be good to have numbers for both cases with
> HA off and HA on.
>
> I'll post these questions to the jira as well.
>
> Thanks,
> --Konstantin
>
> On Wed, Feb 8, 2012 at 9:08 PM, Todd Lipcon  wrote:
>> The branch developed some new conflicts due to recent changes in trunk
>> affecting the RPC between the DN and the NN (the "StorageReport"
>> stuff). I've done a new merge to address these conflicts here:
>>
>> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120208
>>
>> I've also addressed Aaron's comments in the thread above.
>> I ran the unit tests on the branch and they passed.
>>
>> Thanks
>> -Todd
>>
>> On Fri, Feb 3, 2012 at 4:44 PM, Aaron T. Myers  wrote:
>>> Hey Todd,
>>>
>>> The merge largely looks good. I agree with the general approach you took. A
>>> few small comments:
>>>
>>> 1. There's a comment in the OP_ADD case blockabout handling OP_CLOSE. This
>>> makes sense in 0.22/0.23/0.24, but in the HA branch the OP_ADD and OP_CLOSE
>>> cases are completely separate case blocks. I actually find this whole
>>> comment a little confusing, since it numbers the cases we have to handle,
>>> but those numbers aren't referenced anywhere else.
>>>
>>> 2. You mentioned in your message that you don't handle the (invalid) case
>>> of OP_ADD on a new file containing updated blocks, but it looks like the
>>> code actually does, though the code also mentions that we should add a
>>> sanity check that this is actually can't occur. Seems like we should clean
>>> up this inconsistency. I agree that adding asserting this case doesn't
>>> occur is the right way to go.
>>>
>>> 3. If we go with my suggestion in (2), we can also move the call to
>>> FSEditLogLoader#updateBlocks to only the case of OP_ADD for an existing
>>> file, and then get rid of the "INodeFile newFile = oldFile" assignment,
>>> which I found kind of confusing at first. (Though I do see why it's correct
>>> as-implemented.) If you don't go with my suggestion in (2), please add a
>>> comment explaining the assignment.
>>>
>>> Otherwise looks good. Merge away.
>>>
>>> --
>>> Aaron T. Myers
>>> Software Engineer, Cloudera
>>>
>>>
>>>
>>> On Fri, Feb 3, 2012 at 2:10 PM, Todd Lipcon  wrote:
>>>
>>>> I've got a merge pending of trunk into HDFS-1623 -- it was a bit
>>>> complicated so wanted to ask for another set of eyes:
>>>> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120203
>>>> (using github since it's hard to review a merge patch via JIRA)
>>>>
>>>> The interesting bit of the merge was to deal with conflicts with
>>>> HDFS-2718. To summarize the changes I had to make:
>>>> - in the HDFS-1623 branch, we don't deal with the case where OP_ADD
>>>> contains blocks on a new file -- this is a case that doesn't happen on
>>>> real clusters, but currently happens with synthetic logs generated
>>>> from the CreateEditLogs tool. I added a TODO to add a sanity check
>>>> here and will address as a follow-up. Given the difference between
>>>> trunk and branch, there were a couple of small changes that propagated
>>>> into unprotectedAddFile
>>>> - In the HDFS-1623 branch we had already implemented the
>>>> "updateBlocks" call inside FSEditLogLoader. I used that existing
>>>> implementation rather than adding the new one in FSDirectory, since
>>>> this function had some other changes related to HA in the branch
>>>> version.
>>>>
>>>> I'll wait for a +1 before committing. I ran all of the unit tests and
>>>> they passed.
>>>>
>>>> -Todd
>>>> --
>>>> Todd Lipcon
>>>> Software Engineer, Cloudera
>>>>
>>
>>
>>
>> --
>> Todd Lipcon
>> Software Engineer, Cloudera



-- 
Todd Lipcon
Software Engineer, Cloudera

Re: Review request: trunk->HDFS-1623 merge

[Konstantin Shvachko]

I was wondering
1. What was the test plan that has been executed for testing this
implementation of HA? Besides unit tests.
2. Have you done any benchmarks, comparing current cluster performance
against the branch. Would be good to have numbers for both cases with
HA off and HA on.

I'll post these questions to the jira as well.

Thanks,
--Konstantin

On Wed, Feb 8, 2012 at 9:08 PM, Todd Lipcon  wrote:
> The branch developed some new conflicts due to recent changes in trunk
> affecting the RPC between the DN and the NN (the "StorageReport"
> stuff). I've done a new merge to address these conflicts here:
>
> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120208
>
> I've also addressed Aaron's comments in the thread above.
> I ran the unit tests on the branch and they passed.
>
> Thanks
> -Todd
>
> On Fri, Feb 3, 2012 at 4:44 PM, Aaron T. Myers  wrote:
>> Hey Todd,
>>
>> The merge largely looks good. I agree with the general approach you took. A
>> few small comments:
>>
>> 1. There's a comment in the OP_ADD case blockabout handling OP_CLOSE. This
>> makes sense in 0.22/0.23/0.24, but in the HA branch the OP_ADD and OP_CLOSE
>> cases are completely separate case blocks. I actually find this whole
>> comment a little confusing, since it numbers the cases we have to handle,
>> but those numbers aren't referenced anywhere else.
>>
>> 2. You mentioned in your message that you don't handle the (invalid) case
>> of OP_ADD on a new file containing updated blocks, but it looks like the
>> code actually does, though the code also mentions that we should add a
>> sanity check that this is actually can't occur. Seems like we should clean
>> up this inconsistency. I agree that adding asserting this case doesn't
>> occur is the right way to go.
>>
>> 3. If we go with my suggestion in (2), we can also move the call to
>> FSEditLogLoader#updateBlocks to only the case of OP_ADD for an existing
>> file, and then get rid of the "INodeFile newFile = oldFile" assignment,
>> which I found kind of confusing at first. (Though I do see why it's correct
>> as-implemented.) If you don't go with my suggestion in (2), please add a
>> comment explaining the assignment.
>>
>> Otherwise looks good. Merge away.
>>
>> --
>> Aaron T. Myers
>> Software Engineer, Cloudera
>>
>>
>>
>> On Fri, Feb 3, 2012 at 2:10 PM, Todd Lipcon  wrote:
>>
>>> I've got a merge pending of trunk into HDFS-1623 -- it was a bit
>>> complicated so wanted to ask for another set of eyes:
>>> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120203
>>> (using github since it's hard to review a merge patch via JIRA)
>>>
>>> The interesting bit of the merge was to deal with conflicts with
>>> HDFS-2718. To summarize the changes I had to make:
>>> - in the HDFS-1623 branch, we don't deal with the case where OP_ADD
>>> contains blocks on a new file -- this is a case that doesn't happen on
>>> real clusters, but currently happens with synthetic logs generated
>>> from the CreateEditLogs tool. I added a TODO to add a sanity check
>>> here and will address as a follow-up. Given the difference between
>>> trunk and branch, there were a couple of small changes that propagated
>>> into unprotectedAddFile
>>> - In the HDFS-1623 branch we had already implemented the
>>> "updateBlocks" call inside FSEditLogLoader. I used that existing
>>> implementation rather than adding the new one in FSDirectory, since
>>> this function had some other changes related to HA in the branch
>>> version.
>>>
>>> I'll wait for a +1 before committing. I ran all of the unit tests and
>>> they passed.
>>>
>>> -Todd
>>> --
>>> Todd Lipcon
>>> Software Engineer, Cloudera
>>>
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera

Re: Review request: trunk->HDFS-1623 merge

[Todd Lipcon]

The branch developed some new conflicts due to recent changes in trunk
affecting the RPC between the DN and the NN (the "StorageReport"
stuff). I've done a new merge to address these conflicts here:

https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120208

I've also addressed Aaron's comments in the thread above.
I ran the unit tests on the branch and they passed.

Thanks
-Todd

On Fri, Feb 3, 2012 at 4:44 PM, Aaron T. Myers  wrote:
> Hey Todd,
>
> The merge largely looks good. I agree with the general approach you took. A
> few small comments:
>
> 1. There's a comment in the OP_ADD case blockabout handling OP_CLOSE. This
> makes sense in 0.22/0.23/0.24, but in the HA branch the OP_ADD and OP_CLOSE
> cases are completely separate case blocks. I actually find this whole
> comment a little confusing, since it numbers the cases we have to handle,
> but those numbers aren't referenced anywhere else.
>
> 2. You mentioned in your message that you don't handle the (invalid) case
> of OP_ADD on a new file containing updated blocks, but it looks like the
> code actually does, though the code also mentions that we should add a
> sanity check that this is actually can't occur. Seems like we should clean
> up this inconsistency. I agree that adding asserting this case doesn't
> occur is the right way to go.
>
> 3. If we go with my suggestion in (2), we can also move the call to
> FSEditLogLoader#updateBlocks to only the case of OP_ADD for an existing
> file, and then get rid of the "INodeFile newFile = oldFile" assignment,
> which I found kind of confusing at first. (Though I do see why it's correct
> as-implemented.) If you don't go with my suggestion in (2), please add a
> comment explaining the assignment.
>
> Otherwise looks good. Merge away.
>
> --
> Aaron T. Myers
> Software Engineer, Cloudera
>
>
>
> On Fri, Feb 3, 2012 at 2:10 PM, Todd Lipcon  wrote:
>
>> I've got a merge pending of trunk into HDFS-1623 -- it was a bit
>> complicated so wanted to ask for another set of eyes:
>> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120203
>> (using github since it's hard to review a merge patch via JIRA)
>>
>> The interesting bit of the merge was to deal with conflicts with
>> HDFS-2718. To summarize the changes I had to make:
>> - in the HDFS-1623 branch, we don't deal with the case where OP_ADD
>> contains blocks on a new file -- this is a case that doesn't happen on
>> real clusters, but currently happens with synthetic logs generated
>> from the CreateEditLogs tool. I added a TODO to add a sanity check
>> here and will address as a follow-up. Given the difference between
>> trunk and branch, there were a couple of small changes that propagated
>> into unprotectedAddFile
>> - In the HDFS-1623 branch we had already implemented the
>> "updateBlocks" call inside FSEditLogLoader. I used that existing
>> implementation rather than adding the new one in FSDirectory, since
>> this function had some other changes related to HA in the branch
>> version.
>>
>> I'll wait for a +1 before committing. I ran all of the unit tests and
>> they passed.
>>
>> -Todd
>> --
>> Todd Lipcon
>> Software Engineer, Cloudera
>>



-- 
Todd Lipcon
Software Engineer, Cloudera

Re: Review request: trunk->HDFS-1623 merge

[Aaron T. Myers]

On Fri, Feb 3, 2012 at 6:45 PM, Suresh Srinivas wrote:

> Todd, can you please hold off for the merge till Tuesday, until some of the
> other folks working on HA could catch up with some of the recent changes.
>

Note that this merge doesn't have anything to do with any *recent* changes
to the HA branch. The conflicts with HDFS-2718 are from HDFS-2602, which
was committed back in December.

--
Aaron T. Myers
Software Engineer, Cloudera

Re: Review request: trunk->HDFS-1623 merge

[Suresh Srinivas]

Todd, can you please hold off for the merge till Tuesday, until some of the
other folks working on HA could catch up with some of the recent changes.

On Fri, Feb 3, 2012 at 2:10 PM, Todd Lipcon  wrote:

> I've got a merge pending of trunk into HDFS-1623 -- it was a bit
> complicated so wanted to ask for another set of eyes:
> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120203
> (using github since it's hard to review a merge patch via JIRA)
>
> The interesting bit of the merge was to deal with conflicts with
> HDFS-2718. To summarize the changes I had to make:
> - in the HDFS-1623 branch, we don't deal with the case where OP_ADD
> contains blocks on a new file -- this is a case that doesn't happen on
> real clusters, but currently happens with synthetic logs generated
> from the CreateEditLogs tool. I added a TODO to add a sanity check
> here and will address as a follow-up. Given the difference between
> trunk and branch, there were a couple of small changes that propagated
> into unprotectedAddFile
> - In the HDFS-1623 branch we had already implemented the
> "updateBlocks" call inside FSEditLogLoader. I used that existing
> implementation rather than adding the new one in FSDirectory, since
> this function had some other changes related to HA in the branch
> version.
>
> I'll wait for a +1 before committing. I ran all of the unit tests and
> they passed.
>
> -Todd
> --
> Todd Lipcon
> Software Engineer, Cloudera
>

Re: Review request: trunk->HDFS-1623 merge

[Aaron T. Myers]

Hey Todd,

The merge largely looks good. I agree with the general approach you took. A
few small comments:

1. There's a comment in the OP_ADD case blockabout handling OP_CLOSE. This
makes sense in 0.22/0.23/0.24, but in the HA branch the OP_ADD and OP_CLOSE
cases are completely separate case blocks. I actually find this whole
comment a little confusing, since it numbers the cases we have to handle,
but those numbers aren't referenced anywhere else.

2. You mentioned in your message that you don't handle the (invalid) case
of OP_ADD on a new file containing updated blocks, but it looks like the
code actually does, though the code also mentions that we should add a
sanity check that this is actually can't occur. Seems like we should clean
up this inconsistency. I agree that adding asserting this case doesn't
occur is the right way to go.

3. If we go with my suggestion in (2), we can also move the call to
FSEditLogLoader#updateBlocks to only the case of OP_ADD for an existing
file, and then get rid of the "INodeFile newFile = oldFile" assignment,
which I found kind of confusing at first. (Though I do see why it's correct
as-implemented.) If you don't go with my suggestion in (2), please add a
comment explaining the assignment.

Otherwise looks good. Merge away.

--
Aaron T. Myers
Software Engineer, Cloudera



On Fri, Feb 3, 2012 at 2:10 PM, Todd Lipcon  wrote:

> I've got a merge pending of trunk into HDFS-1623 -- it was a bit
> complicated so wanted to ask for another set of eyes:
> https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120203
> (using github since it's hard to review a merge patch via JIRA)
>
> The interesting bit of the merge was to deal with conflicts with
> HDFS-2718. To summarize the changes I had to make:
> - in the HDFS-1623 branch, we don't deal with the case where OP_ADD
> contains blocks on a new file -- this is a case that doesn't happen on
> real clusters, but currently happens with synthetic logs generated
> from the CreateEditLogs tool. I added a TODO to add a sanity check
> here and will address as a follow-up. Given the difference between
> trunk and branch, there were a couple of small changes that propagated
> into unprotectedAddFile
> - In the HDFS-1623 branch we had already implemented the
> "updateBlocks" call inside FSEditLogLoader. I used that existing
> implementation rather than adding the new one in FSDirectory, since
> this function had some other changes related to HA in the branch
> version.
>
> I'll wait for a +1 before committing. I ran all of the unit tests and
> they passed.
>
> -Todd
> --
> Todd Lipcon
> Software Engineer, Cloudera
>

Review request: trunk->HDFS-1623 merge

[Todd Lipcon]

I've got a merge pending of trunk into HDFS-1623 -- it was a bit
complicated so wanted to ask for another set of eyes:
https://github.com/toddlipcon/hadoop-common/tree/ha-merge-20120203
(using github since it's hard to review a merge patch via JIRA)

The interesting bit of the merge was to deal with conflicts with
HDFS-2718. To summarize the changes I had to make:
- in the HDFS-1623 branch, we don't deal with the case where OP_ADD
contains blocks on a new file -- this is a case that doesn't happen on
real clusters, but currently happens with synthetic logs generated
from the CreateEditLogs tool. I added a TODO to add a sanity check
here and will address as a follow-up. Given the difference between
trunk and branch, there were a couple of small changes that propagated
into unprotectedAddFile
- In the HDFS-1623 branch we had already implemented the
"updateBlocks" call inside FSEditLogLoader. I used that existing
implementation rather than adding the new one in FSDirectory, since
this function had some other changes related to HA in the branch
version.

I'll wait for a +1 before committing. I ran all of the unit tests and
they passed.

-Todd
-- 
Todd Lipcon
Software Engineer, Cloudera

Review Request: HDFS-234 Integration with BookKeeper logging system

[Ivan Kelly]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2835/
---

Review request for hadoop-hdfs.


Summary
---

BookKeeper is a system to reliably log streams of records 
(https://issues.apache.org/jira/browse/ZOOKEEPER-276). The NameNode is a 
natural target for such a system for being the metadata repository of the 
entire file system for HDFS. 


This addresses bug HDFS-234.
http://issues.apache.org/jira/browse/HDFS-234


Diffs
-

  hadoop-hdfs-project/hadoop-hdfs/pom.xml 1bcc372 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/bkjournal/BookKeeperEditLogInputStream.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/bkjournal/TestBookKeeperJournalManager.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogTestUtil.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/bkjournal/EditLogLedgerMetadata.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/bkjournal/MaxTxId.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/bkjournal/WriteLock.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/bkjournal/BookKeeperEditLogOutputStream.java
 PRE-CREATION 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/bkjournal/BookKeeperJournalManager.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/2835/diff


Testing
---


Thanks,

Ivan

Re: Review Request: HDFS-1580: Add interface for generic Write Ahead Logging mechanisms

[Ivan Kelly]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2672/
---

(Updated 2011-11-03 11:25:09.526158)


Review request for hadoop-hdfs.


Summary
---

This is the final piece to allow the loading of custom implementations of 
JournalManager. There is another change HDFS-2334 which adds closeable to 
JournalManager, but that may not be absolutely necessary for all journal types. 
(it is for bookkeeper)

There's 2 changes:
1) I've changes the interfaces(JournalManager, EditLogInputStream & 
EditLogOutputStream) so that they can be implemented outside of the 
org.apache.hadoop.hdfs.server.namenode.

2) Pluggable creation of journal managers.
When FSEditLog is creating JournalManagers from dfs.namenode.edits.dir, and it 
encounters a URI with a schema different to "file" it loads the name of the 
implementing class from "dfs.namenode.edits.journal-plugin.". This 
class must implement JournalManager and have a constructor which takes 
(Configuration, URI).


This addresses bug HDFS-1580.
http://issues.apache.org/jira/browse/HDFS-1580


Diffs (updated)
-

  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
 7630335 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
 974697d 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupOutputStream.java
 067990d 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
 9db7f8a 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileOutputStream.java
 4780d04 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java
 c6f8505 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogOutputStream.java
 8681837 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
 f80f863 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
 991fd08 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
 3adb439 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalManager.java
 348e3ef 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalSet.java
 45b5714 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
 a7fa7fb 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeResourceChecker.java
 4d7cfd8 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGenericJournalConf.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/2672/diff


Testing
---


Thanks,

Ivan

Re: Review Request: HDFS-1580: Add interface for generic Write Ahead Logging mechanisms

[Ivan Kelly]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2672/
---

(Updated 2011-11-02 20:54:27.126053)


Review request for hadoop-hdfs.


Summary
---

This is the final piece to allow the loading of custom implementations of 
JournalManager. There is another change HDFS-2334 which adds closeable to 
JournalManager, but that may not be absolutely necessary for all journal types. 
(it is for bookkeeper)

There's 2 changes:
1) I've changes the interfaces(JournalManager, EditLogInputStream & 
EditLogOutputStream) so that they can be implemented outside of the 
org.apache.hadoop.hdfs.server.namenode.

2) Pluggable creation of journal managers.
When FSEditLog is creating JournalManagers from dfs.namenode.edits.dir, and it 
encounters a URI with a schema different to "file" it loads the name of the 
implementing class from "dfs.namenode.edits.journal-plugin.". This 
class must implement JournalManager and have a constructor which takes 
(Configuration, URI).


This addresses bug HDFS-1580.
http://issues.apache.org/jira/browse/HDFS-1580


Diffs (updated)
-

  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
 dd39676 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
 974697d 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupOutputStream.java
 067990d 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
 9db7f8a 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileOutputStream.java
 4780d04 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java
 c6f8505 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogOutputStream.java
 8681837 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
 f80f863 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
 991fd08 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
 3adb439 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalManager.java
 348e3ef 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalSet.java
 45b5714 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
 a7fa7fb 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeResourceChecker.java
 4d7cfd8 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGenericJournalConf.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/2672/diff


Testing
---


Thanks,

Ivan

Re: Review Request: HDFS-1580: Add interface for generic Write Ahead Logging mechanisms

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2672/#review3014
---



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
<https://reviews.apache.org/r/2672/#comment6727>

we use _PREFIX instead of _BASE elsewhere for key prefixes



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
<https://reviews.apache.org/r/2672/#comment6728>

why not just use conf.getClass here and return a Class? And throw the 
exception right here instead of returning null and throwing below



hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeResourceChecker.java
<https://reviews.apache.org/r/2672/#comment6729>

this is the wrong layer - better to filter for file:// URLs where this is 
called, I think.



hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGenericJournalConf.java
<https://reviews.apache.org/r/2672/#comment6730>

no need to have any datanodes for any of these tests - will run faster 
without.



hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGenericJournalConf.java
<https://reviews.apache.org/r/2672/#comment6731>

our convention is to use american spelling (initialized)



hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGenericJournalConf.java
<https://reviews.apache.org/r/2672/#comment6732>

our style is to not have multiple classes per .java file unless they're 
inner classes. You can make this a static inner class of the test.



hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGenericJournalConf.java
<https://reviews.apache.org/r/2672/#comment6733>

just return Mockito.mock(EditLogOutputStream.class) and you don't need to 
have the whole implementation below


- Todd


On 2011-11-02 14:33:47, Ivan Kelly wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2672/
> -------
> 
> (Updated 2011-11-02 14:33:47)
> 
> 
> Review request for hadoop-hdfs.
> 
> 
> Summary
> ---
> 
> This is the final piece to allow the loading of custom implementations of 
> JournalManager. There is another change HDFS-2334 which adds closeable to 
> JournalManager, but that may not be absolutely necessary for all journal 
> types. (it is for bookkeeper)
> 
> There's 2 changes:
> 1) I've changes the interfaces(JournalManager, EditLogInputStream & 
> EditLogOutputStream) so that they can be implemented outside of the 
> org.apache.hadoop.hdfs.server.namenode.
> 
> 2) Pluggable creation of journal managers.
> When FSEditLog is creating JournalManagers from dfs.namenode.edits.dir, and 
> it encounters a URI with a schema different to "file" it loads the name of 
> the implementing class from "dfs.namenode.edits.journal-plugin.". 
> This class must implement JournalManager and have a constructor which takes 
> (Configuration, URI).
> 
> 
> This addresses bug HDFS-1580.
> http://issues.apache.org/jira/browse/HDFS-1580
> 
> 
> Diffs
> -
> 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
>  dd39676 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
>  974697d 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupOutputStream.java
>  067990d 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
>  9db7f8a 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileOutputStream.java
>  4780d04 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java
>  c6f8505 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogOutputStream.java
>  8681837 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
>  f80f863 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
>  991fd08 
>   
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
>  3adb439 
>   
> hadoop-hdfs-proje

Review Request: HDFS-1580: Add interface for generic Write Ahead Logging mechanisms

[Ivan Kelly]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2672/
---

Review request for hadoop-hdfs.


Summary
---

This is the final piece to allow the loading of custom implementations of 
JournalManager. There is another change HDFS-2334 which adds closeable to 
JournalManager, but that may not be absolutely necessary for all journal types. 
(it is for bookkeeper)

There's 2 changes:
1) I've changes the interfaces(JournalManager, EditLogInputStream & 
EditLogOutputStream) so that they can be implemented outside of the 
org.apache.hadoop.hdfs.server.namenode.

2) Pluggable creation of journal managers.
When FSEditLog is creating JournalManagers from dfs.namenode.edits.dir, and it 
encounters a URI with a schema different to "file" it loads the name of the 
implementing class from "dfs.namenode.edits.journal-plugin.". This 
class must implement JournalManager and have a constructor which takes 
(Configuration, URI).


This addresses bug HDFS-1580.
http://issues.apache.org/jira/browse/HDFS-1580


Diffs
-

  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
 dd39676 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupInputStream.java
 974697d 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogBackupOutputStream.java
 067990d 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileInputStream.java
 9db7f8a 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogFileOutputStream.java
 4780d04 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogInputStream.java
 c6f8505 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/EditLogOutputStream.java
 8681837 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
 f80f863 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogLoader.java
 991fd08 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLogOp.java
 3adb439 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalManager.java
 348e3ef 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalSet.java
 45b5714 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NNStorage.java
 a7fa7fb 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNodeResourceChecker.java
 4d7cfd8 
  
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestGenericJournalConf.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/2672/diff


Testing
---


Thanks,

Ivan

Re: Review Request: HDFS-2334: Add Closeable to JournalManager

[Ivan Kelly]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2247/
---

(Updated 2011-10-24 09:18:46.680787)


Review request for hadoop-hdfs.


Changes
---

Added check for null on JournalAndStream#close


Summary
---

A JournalManager may take hold of resources for the duration of their lifetime. 
This isn't the case at the moment for FileJournalManager, but 
BookKeeperJournalManager will, and it's conceivable that FileJournalManager 
could take a lock on a directory etc.

This JIRA is to add Closeable to JournalManager so that these resources can be 
cleaned up when FSEditLog is closed.


This addresses bug HDFS-2334.
http://issues.apache.org/jira/browse/HDFS-2334


Diffs (updated)
-

  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
 aac2a35 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java
 8cfc975 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalManager.java
 0bb7b0f 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupJournalManager.java
 6976620 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalSet.java
 0d6bc74 

Diff: https://reviews.apache.org/r/2247/diff


Testing
---


Thanks,

Ivan

Re: Review Request: HDFS-2334: Add Closeable to JournalManager

[Ivan Kelly]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2247/
---

(Updated 2011-10-20 09:00:57.815922)


Review request for hadoop-hdfs.


Summary (updated)
---

A JournalManager may take hold of resources for the duration of their lifetime. 
This isn't the case at the moment for FileJournalManager, but 
BookKeeperJournalManager will, and it's conceivable that FileJournalManager 
could take a lock on a directory etc.

This JIRA is to add Closeable to JournalManager so that these resources can be 
cleaned up when FSEditLog is closed.


This addresses bug HDFS-2334.
http://issues.apache.org/jira/browse/HDFS-2334


Diffs
-

  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupJournalManager.java
 6976620 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
 4a41a2c 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java
 8cfc975 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalManager.java
 0bb7b0f 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalSet.java
 0d6bc74 

Diff: https://reviews.apache.org/r/2247/diff


Testing
---


Thanks,

Ivan

Review Request for HDFS-140

[Uma Maheswara Rao G 72686]

Hi,

 Provided the patch for HDFS-140. If you have some free time, can you please 
review it?

 HDFS-140 (When a file is deleted, its blocks remain in the blocksmap till the 
next block report from Datanode)
 It will give advantage in terms of memory usage at NN side.


Thanks & Regards,
Uma

Re: Review Request: HDFS-2301 Start/stop appropriate namenode internal services during transition to active and standby

[Suresh Srinivas]

> On 2011-10-06 23:37:24, Todd Lipcon wrote:
> > branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HAUtil.java,
> >  line 28
> > <https://reviews.apache.org/r/2150/diff/2/?file=48551#file48551line28>
> >
> > perhaps should be abstract since it won't ever be instantiated?
> > 
> > these functions are meant only for the server side, right? Otherwise 
> > they should all take an authority, and look at configs prefixed/suffixed 
> > with that authority?
> > 
> > let me jump over to HDFS-2231 and try to review that first.. having a 
> > hard time following this.

Class has private constructor. I prefer that to abstract class. For some 
reason, I included HAUtil in this patch causing lot of confusion. It belongs to 
2231. So I am attaching a patch without HAUtil changes. Also I will address 
rest of your HAUtil comments in 2231.


- Suresh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2150/#review2422
---


On 2011-10-06 23:25:18, Todd Lipcon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2150/
> -----------
> 
> (Updated 2011-10-06 23:25:18)
> 
> 
> Review request for hadoop-hdfs and Todd Lipcon.
> 
> 
> Summary
> ---
> 
> Uploading Suresh's patch to reviewboard 
> (https://issues.apache.org/jira/secure/attachment/12496953/HDFS-2301.txt from 
> 29/Sep/11 00:56)
> 
> 
> This addresses bug HDFS-2301.
> https://issues.apache.org/jira/browse/HDFS-2301
> 
> 
> Diffs
> -
> 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HAUtil.java
>  PRE-CREATION 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupNode.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/ActiveState.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/HAContext.java
>  PRE-CREATION 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/HAState.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/StandbyState.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestHAUtil.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/2150/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Todd
> 
>

Re: Review Request: HDFS-2301 Start/stop appropriate namenode internal services during transition to active and standby

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2150/#review2422
---



branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HAUtil.java
<https://reviews.apache.org/r/2150/#comment5514>

perhaps should be abstract since it won't ever be instantiated?

these functions are meant only for the server side, right? Otherwise they 
should all take an authority, and look at configs prefixed/suffixed with that 
authority?

let me jump over to HDFS-2231 and try to review that first.. having a hard 
time following this.



branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HAUtil.java
<https://reviews.apache.org/r/2150/#comment5515>

!collection.isEmpty()



branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HAUtil.java
<https://reviews.apache.org/r/2150/#comment5516>

long line



branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HAUtil.java
<https://reviews.apache.org/r/2150/#comment5518>

strange formatting



branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HAUtil.java
<https://reviews.apache.org/r/2150/#comment5517>

so this patch now depends on HDFS-2231 (Conf changes for HA NN), right?


- Todd


On 2011-10-06 23:25:18, Todd Lipcon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2150/
> ---
> 
> (Updated 2011-10-06 23:25:18)
> 
> 
> Review request for hadoop-hdfs and Todd Lipcon.
> 
> 
> Summary
> ---
> 
> Uploading Suresh's patch to reviewboard 
> (https://issues.apache.org/jira/secure/attachment/12496953/HDFS-2301.txt from 
> 29/Sep/11 00:56)
> 
> 
> This addresses bug HDFS-2301.
> https://issues.apache.org/jira/browse/HDFS-2301
> 
> 
> Diffs
> -
> 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HAUtil.java
>  PRE-CREATION 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupNode.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/ActiveState.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/HAContext.java
>  PRE-CREATION 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/HAState.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/StandbyState.java
>  1179521 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestHAUtil.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/2150/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Todd
> 
>

Re: Review Request: HDFS-2301 Start/stop appropriate namenode internal services during transition to active and standby

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2150/
---

(Updated 2011-10-06 23:25:18.391776)


Review request for hadoop-hdfs and Todd Lipcon.


Changes
---

updating reviewboard patch to Suresh's new revision


Summary
---

Uploading Suresh's patch to reviewboard 
(https://issues.apache.org/jira/secure/attachment/12496953/HDFS-2301.txt from 
29/Sep/11 00:56)


This addresses bug HDFS-2301.
https://issues.apache.org/jira/browse/HDFS-2301


Diffs (updated)
-

  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/HAUtil.java
 PRE-CREATION 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupNode.java
 1179521 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1179521 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1179521 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/ActiveState.java
 1179521 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/HAContext.java
 PRE-CREATION 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/HAState.java
 1179521 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/StandbyState.java
 1179521 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestHAUtil.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/2150/diff


Testing
---


Thanks,

Todd

Re: Review Request: HDFS-2301 Start/stop appropriate namenode internal services during transition to active and standby

[Todd Lipcon]

> On 2011-10-03 18:58:13, Todd Lipcon wrote:
> > just a few nits, mostly looks good. A few questions I have that aren't 
> > directly related to this patch:
> > - is SafeMode now a replicated thing, or does each NN separately enter 
> > safemode? I think the latter, right?
> > - when transitioning between states, what happens if the "enterState" fails 
> > for the new state? The state variable will then indicate it's in that 
> > state, when in fact it's in no state at all. How do we recover from that? 
> > We need some kind of rollback? (eg if you're in standby and try to 
> > transition to active, but find that you can't take a lock in ZK)
> 
> Suresh Srinivas wrote:
> > is SafeMode now a replicated thing, or does each NN separately enter 
> safemode? I think the latter, right?
> Safemode is the state of namespace(FSNamesystem), unlike active, standby 
> which are the states of the namenode. Each NN separately enters safemode.
> 
> > when transitioning between states, what happens if the "enterState" 
> fails for the new state? The state variable will then indicate it's in that 
> state, when in fact it's in no state at all. How do we recover from that? We 
> need some kind of rollback? (eg if you're in standby and try to transition to 
> active, but find that you can't take a lock in ZK)
> This is tricky. Say enterState fails to start services because of some 
> namenode process related issues. Then most likely rolling back to previous 
> state, and starting services relevant to previous states will also fail. The 
> particular example you are bringing up related to ZK, I think failover 
> controller is the one that deals with ZK and not namenode.
> 
> I can think of two solutions: namenode shutsdown when this happens (as 
> done during startup) or move to a failed state.

Let's just add a TODO for now that we need to consider these situations in a 
test plan. I imagine the most likely real scenario is that you try to do a 
failover, but for some reason the standby has an IO problem trying to read the 
latest logs from the primary (eg maybe the primary barfed some bad data into 
the edit logs as it crashed, or maybe the primary crashed because the shared 
storage caught on fire).


> On 2011-10-03 18:58:13, Todd Lipcon wrote:
> > branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java,
> >  line 464
> > <https://reviews.apache.org/r/2150/diff/1/?file=47529#file47529line464>
> >
> > any reason that you switched the order of startHttpServer to the end of 
> > this function? I don't think it's a big deal, but there's some possibility 
> > the service plugins may want to do something with the http server, which 
> > wouldn't be started yet.
> 
> Suresh Srinivas wrote:
> No particular reason. Not sure who uses ServicePlugins. But the 
> description says it is RPC related. But will move it back up.

Hue currently uses service plugins to expose a Thrift interface. But with 
Sanjay's recent work on protocol adapters, this may be largely unnecessary in 
the future. Nonetheless, we should leave it around :)


- Todd


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2150/#review2277
---


On 2011-10-03 18:36:41, Todd Lipcon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2150/
> ---
> 
> (Updated 2011-10-03 18:36:41)
> 
> 
> Review request for hadoop-hdfs and Todd Lipcon.
> 
> 
> Summary
> ---
> 
> Uploading Suresh's patch to reviewboard 
> (https://issues.apache.org/jira/secure/attachment/12496953/HDFS-2301.txt from 
> 29/Sep/11 00:56)
> 
> 
> This addresses bug HDFS-2301.
> https://issues.apache.org/jira/browse/HDFS-2301
> 
> 
> Diffs
> -
> 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupNode.java
>  1177130 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
>  1177130 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
>  1177130 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/ActiveSta

Review Request: Add Closeable to JournalManager

[Ivan Kelly]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2247/
---

Review request for hadoop-hdfs.


Summary
---

A JournalManager may take hold of resources for the duration of their lifetime. 
This isn't the case at the moment for FileJournalManager, but 
BookKeeperJournalManager will, and it's conceivable that FileJournalManager 
could take a lock on a directory etc.

This JIRA is to add Closeable to JournalManager so that these resources can be 
cleaned up when FSEditLog is closed.


This addresses bug HDFS-2334.
http://issues.apache.org/jira/browse/HDFS-2334


Diffs
-

  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupJournalManager.java
 6976620 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSEditLog.java
 4a41a2c 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FileJournalManager.java
 8cfc975 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalManager.java
 0bb7b0f 
  
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/JournalSet.java
 0d6bc74 

Diff: https://reviews.apache.org/r/2247/diff


Testing
---


Thanks,

Ivan

Re: Review Request: HDFS-2301 Start/stop appropriate namenode internal services during transition to active and standby

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2150/#review2277
---


just a few nits, mostly looks good. A few questions I have that aren't directly 
related to this patch:
- is SafeMode now a replicated thing, or does each NN separately enter 
safemode? I think the latter, right?
- when transitioning between states, what happens if the "enterState" fails for 
the new state? The state variable will then indicate it's in that state, when 
in fact it's in no state at all. How do we recover from that? We need some kind 
of rollback? (eg if you're in standby and try to transition to active, but find 
that you can't take a lock in ZK)


branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
<https://reviews.apache.org/r/2150/#comment5248>

typo: secrect



branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
<https://reviews.apache.org/r/2150/#comment5249>

any reason that you switched the order of startHttpServer to the end of 
this function? I don't think it's a big deal, but there's some possibility the 
service plugins may want to do something with the http server, which wouldn't 
be started yet.



branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
<https://reviews.apache.org/r/2150/#comment5251>

this should at least be warn, right? and passing e as the second argument 
is better since it shows the trace.


- Todd


On 2011-10-03 18:36:41, Todd Lipcon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/2150/
> -----------
> 
> (Updated 2011-10-03 18:36:41)
> 
> 
> Review request for hadoop-hdfs and Todd Lipcon.
> 
> 
> Summary
> ---
> 
> Uploading Suresh's patch to reviewboard 
> (https://issues.apache.org/jira/secure/attachment/12496953/HDFS-2301.txt from 
> 29/Sep/11 00:56)
> 
> 
> This addresses bug HDFS-2301.
> https://issues.apache.org/jira/browse/HDFS-2301
> 
> 
> Diffs
> -
> 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupNode.java
>  1177130 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
>  1177130 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
>  1177130 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/ActiveState.java
>  1177128 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/HAContext.java
>  PRE-CREATION 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/HAState.java
>  1177128 
>   
> branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/StandbyState.java
>  1177128 
> 
> Diff: https://reviews.apache.org/r/2150/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Todd
> 
>

Review Request: HDFS-2301 Start/stop appropriate namenode internal services during transition to active and standby

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2150/
---

Review request for hadoop-hdfs and Todd Lipcon.


Summary
---

Uploading Suresh's patch to reviewboard 
(https://issues.apache.org/jira/secure/attachment/12496953/HDFS-2301.txt from 
29/Sep/11 00:56)


This addresses bug HDFS-2301.
https://issues.apache.org/jira/browse/HDFS-2301


Diffs
-

  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/BackupNode.java
 1177130 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1177130 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1177130 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/ActiveState.java
 1177128 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/HAContext.java
 PRE-CREATION 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/HAState.java
 1177128 
  
branches/HDFS-1623/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/ha/StandbyState.java
 1177128 

Diff: https://reviews.apache.org/r/2150/diff


Testing
---


Thanks,

Todd

Re: Review Request: HDFS-395 DFS Scalability: Incremental block reports

[Tomasz Nykiel]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/998/
---

(Updated 2011-08-23 22:55:47.297820)


Review request for hadoop-hdfs, Dhruba Borthakur and Hairong Kuang.


Changes
---

Adjusted to recent code changes.
Fized offerService comment.


Summary
---

This patch introduces explicit ACKs sent form datanodes to the namenode in 
order to minimize the difference between NN blocks info and DN state.
This will allow for sending the full block reports less frequently, which in 
turn will minimize the overhead of processing them, and blocking the namenode 
for extended time.


This addresses bug HDFS-395.
https://issues.apache.org/jira/browse/HDFS-395


Diffs (updated)
-

  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
 1160919 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
 1160919 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/FSDataset.java
 1160919 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/FSDatasetAsyncDiskService.java
 1160919 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1160919 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockCommand.java
 1160919 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/DatanodeProtocol.java
 1160919 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/ReceivedDeletedBlockInfo.java
 PRE-CREATION 
  
trunk/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java
 1160919 
  
trunk/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDeadDatanode.java
 1160919 

Diff: https://reviews.apache.org/r/998/diff


Testing
---

Will do JUnit tests.


Thanks,

Tomasz

Re: Review Request: HDFS-395 DFS Scalability: Incremental block reports

[Tomasz Nykiel]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/998/
---

(Updated 2011-08-24 03:33:32.850921)


Review request for hadoop-hdfs, Dhruba Borthakur and Hairong Kuang.


Summary
---

This patch introduces explicit ACKs sent form datanodes to the namenode in 
order to minimize the difference between NN blocks info and DN state.
This will allow for sending the full block reports less frequently, which in 
turn will minimize the overhead of processing them, and blocking the namenode 
for extended time.


This addresses bug HDFS-395.
https://issues.apache.org/jira/browse/HDFS-395


Diffs (updated)
-

  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
 1160950 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
 1160950 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/FSDataset.java
 1160950 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/datanode/FSDatasetAsyncDiskService.java
 1160950 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1160950 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/BlockCommand.java
 1160950 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/DatanodeProtocol.java
 1160950 
  
trunk/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/protocol/ReceivedDeletedBlockInfo.java
 PRE-CREATION 
  
trunk/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java
 1160950 
  
trunk/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDeadDatanode.java
 1160950 

Diff: https://reviews.apache.org/r/998/diff


Testing
---

Will do JUnit tests.


Thanks,

Tomasz

Re: Review Request: HDFS-395 DFS Scalability: Incremental block reports

[Tomasz Nykiel]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/998/
---

(Updated 2011-07-28 00:40:14.000447)


Review request for hadoop-hdfs, Dhruba Borthakur and Hairong Kuang.


Changes
---

Addressed the comments.


Summary
---

This patch introduces explicit ACKs sent form datanodes to the namenode in 
order to minimize the difference between NN blocks info and DN state.
This will allow for sending the full block reports less frequently, which in 
turn will minimize the overhead of processing them, and blocking the namenode 
for extended time.


This addresses bug HDFS-395.
https://issues.apache.org/jira/browse/HDFS-395


Diffs (updated)
-

  
trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
 1151690 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java 
1151690 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/FSDataset.java 
1151690 
  
trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/FSDatasetAsyncDiskService.java
 1151690 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java 
1151690 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java 
1151690 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/BlockCommand.java 
1151690 
  
trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/DatanodeProtocol.java
 1151690 
  
trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/ReceivedDeletedBlockInfo.java
 PRE-CREATION 
  
trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java
 1151690 
  
trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestDeadDatanode.java
 1151690 

Diff: https://reviews.apache.org/r/998/diff


Testing
---

Will do JUnit tests.


Thanks,

Tomasz

Re: Review Request: HDFS-395 DFS Scalability: Incremental block reports

[Tomasz Nykiel]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/998/
---

(Updated 2011-07-11 21:06:02.473171)


Review request for hadoop-hdfs, Dhruba Borthakur and Hairong Kuang.


Changes
---

- I removed renaming of the block and meta file from FSDataset. We should 
consider it in a separate diff, as suggested by Hairong. By doing so, I need to 
defer the notification until the FSAsyncDiskService deletes the files.
- I added an optimization, which does not procuce ACKs for blocks which come 
from entirely deleted files (in this situation, the blocks are instantly 
removed from the blocks map at the NN side, and hence we need no ACK).


Summary
---

This patch introduces explicit ACKs sent form datanodes to the namenode in 
order to minimize the difference between NN blocks info and DN state.
This will allow for sending the full block reports less frequently, which in 
turn will minimize the overhead of processing them, and blocking the namenode 
for extended time.


This addresses bug HDFS-395.
https://issues.apache.org/jira/browse/HDFS-395


Diffs (updated)
-

  
trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/blockmanagement/BlockManager.java
 1145346 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java 
1145346 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/FSDataset.java 
1145346 
  
trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/FSDatasetAsyncDiskService.java
 1145346 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java 
1145346 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java 
1145346 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/BlockCommand.java 
1145346 
  
trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/DatanodeProtocol.java
 1145346 
  
trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/ReceivedDeletedBlockInfo.java
 PRE-CREATION 
  
trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java
 1145346 
  
trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestDeadDatanode.java
 1145346 

Diff: https://reviews.apache.org/r/998/diff


Testing
---

Will do JUnit tests.


Thanks,

Tomasz

Re: Review Request: HDFS-395 DFS Scalability: Incremental block reports

[Tomasz Nykiel]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/998/
---

(Updated 2011-07-01 20:34:17.116645)


Review request for hadoop-hdfs, Dhruba Borthakur and Hairong Kuang.


Summary
---

This patch introduces explicit ACKs sent form datanodes to the namenode in 
order to minimize the difference between NN blocks info and DN state.
This will allow for sending the full block reports less frequently, which in 
turn will minimize the overhead of processing them, and blocking the namenode 
for extended time.


This addresses bug HDFS-395.
https://issues.apache.org/jira/browse/HDFS-395


Diffs (updated)
-

  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java 
1142085 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/FSDataset.java 
1142085 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java 
1142085 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java 
1142085 
  
trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/DatanodeProtocol.java
 1142085 
  
trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/ReceivedDeletedBlockInfo.java
 PRE-CREATION 
  
trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java
 1142085 
  
trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestDeadDatanode.java
 1142085 

Diff: https://reviews.apache.org/r/998/diff


Testing
---

Will do JUnit tests.


Thanks,

Tomasz

Review Request: HDFS-395 DFS Scalability: Incremental block reports

[Tomasz Nykiel]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/998/
---

Review request for hadoop-hdfs, Dhruba Borthakur and Hairong Kuang.


Summary
---

This patch introduces explicit ACKs sent form datanodes to the namenode in 
order to minimize the difference between NN blocks info and DN state.
This will allow for sending the full block reports less frequently, which in 
turn will minimize the overhead of processing them, and blocking the namenode 
for extended time.


This addresses bug HDFS-395.
https://issues.apache.org/jira/browse/HDFS-395


Diffs
-

  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java 
1142055 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/datanode/FSDataset.java 
1142055 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java 
1142055 
  trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java 
1142055 
  
trunk/hdfs/src/java/org/apache/hadoop/hdfs/server/protocol/DatanodeProtocol.java
 1142055 
  
trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java
 1142055 
  
trunk/hdfs/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestDeadDatanode.java
 1142055 

Diff: https://reviews.apache.org/r/998/diff


Testing
---

Will do JUnit tests.


Thanks,

Tomasz

Re: Review Request: Misc improvements to HDFS HTML

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/754/#review682
---


a few preliminary comments. I also want to load this up and see how it looks :)


trunk/src/java/org/apache/hadoop/hdfs/server/common/JspHelper.java
<https://reviews.apache.org/r/754/#comment1362>

these lines are missing  and  still



trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NamenodeJspHelper.java
<https://reviews.apache.org/r/754/#comment1363>

missing  on this line and lines below



trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NamenodeJspHelper.java
<https://reviews.apache.org/r/754/#comment1364>

do we need warning class on both the div and the a?



trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NamenodeJspHelper.java
<https://reviews.apache.org/r/754/#comment1365>

can we kill off these table attributes and use CSS styles to style all the 
tables consistently?


- Todd


On 2011-05-18 19:17:02, Todd Lipcon wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/754/
> ---
> 
> (Updated 2011-05-18 19:17:02)
> 
> 
> Review request for hadoop-hdfs, Todd Lipcon and Eugene Koontz.
> 
> 
> Summary
> ---
> 
> Uploading Eugene's patch 
> https://issues.apache.org/jira/secure/attachment/12479631/HDFS-1013.patch for 
> easier review
> 
> 
> This addresses bug HDFS-1013.
> https://issues.apache.org/jira/browse/HDFS-1013
> 
> 
> Diffs
> -
> 
>   trunk/src/java/org/apache/hadoop/hdfs/server/common/JspHelper.java 1124363 
>   
> trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NamenodeJspHelper.java 
> 1124363 
>   trunk/src/webapps/hdfs/dfshealth.jsp 1124363 
>   trunk/src/webapps/static/hadoop.css PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/754/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Todd
> 
>

Review Request: Misc improvements to HDFS HTML

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/754/
---

Review request for hadoop-hdfs, Todd Lipcon and Eugene Koontz.


Summary
---

Uploading Eugene's patch 
https://issues.apache.org/jira/secure/attachment/12479631/HDFS-1013.patch for 
easier review


This addresses bug HDFS-1013.
https://issues.apache.org/jira/browse/HDFS-1013


Diffs
-

  trunk/src/java/org/apache/hadoop/hdfs/server/common/JspHelper.java 1124363 
  trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NamenodeJspHelper.java 
1124363 
  trunk/src/webapps/hdfs/dfshealth.jsp 1124363 
  trunk/src/webapps/static/hadoop.css PRE-CREATION 

Diff: https://reviews.apache.org/r/754/diff


Testing
---


Thanks,

Todd

Re: Review Request: Make exiting safemode a lot faster.

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/196/#review149
---


I think you attached the wrong patch? This seems like the patch for HDFS-1508?

- Todd


On 2011-01-23 20:24:11, Dhruba Borthakur wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/196/
> ---
> 
> (Updated 2011-01-23 20:24:11)
> 
> 
> Review request for hadoop-hdfs.
> 
> 
> Summary
> ---
> 
> Make exiting safemode a lot faster.
> 
> 
> This addresses bug HDFS-1391.
> https://issues.apache.org/jira/browse/HDFS-1391
> 
> 
> Diffs
> -
> 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
>  1062627 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
>  1062627 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
>  1062627 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
>  1062627 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
>  1062627 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java
>  1062627 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/UpgradeUtilities.java
>  1062627 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogRace.java
>  1062627 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestParallelImageWrite.java
>  1062627 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestSaveNamespace.java
>  1062627 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
>  1062627 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/tools/offlineImageViewer/TestOfflineImageViewer.java
>  1062627 
> 
> Diff: https://reviews.apache.org/r/196/diff
> 
> 
> Testing
> ---
> 
> Running on our biggest production cluster of 3000 nodes since Dec 13th
> 
> 
> Thanks,
> 
> Dhruba
> 
>

Review Request: Allow a datanode to copy a block to a datanode on a foreign HDFS cluster

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/346/
---

Review request for hadoop-hdfs.


Summary
---

This patch introduces an RPC to the datanode to allow it to copy a block to a 
datanode on a remote HDFS cluster.


This addresses bug HDFS-1593.
https://issues.apache.org/jira/browse/HDFS-1593


Diffs
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientDatanodeProtocol.java
 1062667 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
 1062667 

Diff: https://reviews.apache.org/r/346/diff


Testing
---


Thanks,

Dhruba

Re: Review Request: Make exiting safemode a lot faster.

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/196/
---

(Updated 2011-01-23 20:24:11.634622)


Review request for hadoop-hdfs.


Changes
---

Addressed review comments.


Summary
---

Make exiting safemode a lot faster.


This addresses bug HDFS-1391.
https://issues.apache.org/jira/browse/HDFS-1391


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
 1062627 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
 1062627 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
 1062627 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1062627 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1062627 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java
 1062627 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/UpgradeUtilities.java
 1062627 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogRace.java
 1062627 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestParallelImageWrite.java
 1062627 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestSaveNamespace.java
 1062627 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
 1062627 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/tools/offlineImageViewer/TestOfflineImageViewer.java
 1062627 

Diff: https://reviews.apache.org/r/196/diff


Testing
---

Running on our biggest production cluster of 3000 nodes since Dec 13th


Thanks,

Dhruba

Re: Review Request: Make exiting safemode a lot faster.

[Todd Lipcon]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/196/#review83
---



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
<https://reviews.apache.org/r/196/#comment146>

maybe assert namesystem.hasWriteLock() here? I think it's all correct, but 
provides good documentation.



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
<https://reviews.apache.org/r/196/#comment144>

should probably note that these are "out" parameters



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
<https://reviews.apache.org/r/196/#comment147>

excessReplicateTmp.isEmpty()?



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
<https://reviews.apache.org/r/196/#comment148>

!originalDatanodes.isEmpty()



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
<https://reviews.apache.org/r/196/#comment149>

can you keep this as a separate addToExcessReplicate function?



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
<https://reviews.apache.org/r/196/#comment151>

add back the isDebugEnabled guard?



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
<https://reviews.apache.org/r/196/#comment150>

this is just moved code, but this comment should say "instructions to the 
datanode" right?



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
<https://reviews.apache.org/r/196/#comment152>

this is already logged inside addToInvalidates. Perhaps pass third argument 
false to that function so it doesn't log, if you want the specialized message?



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
<https://reviews.apache.org/r/196/#comment145>

update javadoc to explain excessReplicateMapTmp is where the results end up

Also maybe worth noting here and in the replicator interface that 'inode' 
here might get modified by another thread during execution?


- Todd


On 2010-12-28 11:12:46, Dhruba Borthakur wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/196/
> -------
> 
> (Updated 2010-12-28 11:12:46)
> 
> 
> Review request for hadoop-hdfs.
> 
> 
> Summary
> ---
> 
> Make exiting safemode a lot faster.
> 
> 
> This addresses bug HDFS-1391.
> https://issues.apache.org/jira/browse/HDFS-1391
> 
> 
> Diffs
> -
> 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
>  1053405 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
>  1053405 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestOverReplicatedBlocks.java
>  1053405 
> 
> Diff: https://reviews.apache.org/r/196/diff
> 
> 
> Testing
> ---
> 
> Running on our biggest production cluster of 3000 nodes since Dec 13th
> 
> 
> Thanks,
> 
> Dhruba
> 
>

Re: Review Request: Make Datanode handle errors to namenode.register call more elegantly

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/195/
---

(Updated 2010-12-28 14:09:46.364223)


Review request for hadoop-hdfs.


Summary
---

Make Datanode handle errors to namenode.register call more elegantly


This addresses bug HDFS-1540.
https://issues.apache.org/jira/browse/HDFS-1540


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
 1053403 

Diff: https://reviews.apache.org/r/195/diff


Testing
---

Running on our clusters for about one month now


Thanks,

Dhruba

Re: Review Request: Make Datanode handle errors to namenode.register call more elegantly

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/195/
---

(Updated 2010-12-28 13:55:18.599061)


Review request for hadoop-hdfs.


Changes
---

Incorporated Konstantin's review comments.


Summary
---

Make Datanode handle errors to namenode.register call more elegantly


This addresses bug HDFS-1540.
https://issues.apache.org/jira/browse/HDFS-1540


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
 1053403 

Diff: https://reviews.apache.org/r/195/diff


Testing
---

Running on our clusters for about one month now


Thanks,

Dhruba

Review Request: Make exiting safemode a lot faster.

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/196/
---

Review request for hadoop-hdfs.


Summary
---

Make exiting safemode a lot faster.


This addresses bug HDFS-1391.
https://issues.apache.org/jira/browse/HDFS-1391


Diffs
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
 1053405 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1053405 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestOverReplicatedBlocks.java
 1053405 

Diff: https://reviews.apache.org/r/196/diff


Testing
---

Running on our biggest production cluster of 3000 nodes since Dec 13th


Thanks,

Dhruba

Review Request: Make Datanode handle errors to namenode.register call more elegantly

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/195/
---

Review request for hadoop-hdfs.


Summary
---

Make Datanode handle errors to namenode.register call more elegantly


This addresses bug HDFS-1540.
https://issues.apache.org/jira/browse/HDFS-1540


Diffs
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/datanode/DataNode.java
 1053403 

Diff: https://reviews.apache.org/r/195/diff


Testing
---

Running on our clusters for about one month now


Thanks,

Dhruba

Re: Review Request: Populate needed replication queues before leaving safe mode.

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/105/
---

(Updated 2010-12-09 19:52:13.188412)


Review request for hadoop-hdfs.


Changes
---

- Updated patch to apply to current trunk.
- In BlockManager.markBlockAsCorrupt only update needed replication queues if 
they have been initialized


Summary
---

This patch introduces a new configuration variable 
dfs.namenode.replqueue.threshold-pct that determines the fraction of blocks for 
which block reports have to be received before the NameNode will start 
initializing the needed replication queues. Once a sufficient number of block 
reports have been received, the queues are initialized while the NameNode is 
still in safe mode. After the queues are initialized, subsequent block reports 
are handled by updating the queues incrementally.

The benefit of this is twofold:
- It allows us to compute the replication queues while we are waiting for the 
last few block reports (when the NameNode is mostly idle). Once these block 
reports have been received, we can then immediately leave safe mode without 
having to wait for the computation of the needed replication queues (which 
requires a full traversal of the blocks map).
- With Raid, it may not be necessary to stay in safe mode until all blocks have 
been reported. Using this change, we could monitor if all of the missing blocks 
can be recreated using parity information and if so leave safe mode early. In 
order for this monitoring to work, we need access to the needed replication 
queues while the NameNode is still in safe mode.


This addresses bug HDFS-1476.
https://issues.apache.org/jira/browse/HDFS-1476


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
 1044182 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
 1044182 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1044182 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/MiniDFSCluster.java
 1044182 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
 1044182 

Diff: https://reviews.apache.org/r/105/diff


Testing
---

new test case in TestListCorruptFileBlocks


Thanks,

Patrick

Re: Review Request: Ability to do savenamespace without being in safemode

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/125/
---

(Updated 2010-12-07 11:01:28.866525)


Review request for hadoop-hdfs.


Changes
---

Incorporated Hairong's review comments.


Summary
---

The namenode need not be in safemode while runnign the saveNamespace command. 
The saveNamespace command acquires the FSNamesystem writelock, thus preventing 
anybody else from modifying the namespace.

The lease expiry thread in the LeaseManager acquires the FSNamesystem-writelock 
too, so it is well protected.


This addresses bug HDFS-1508.
https://issues.apache.org/jira/browse/HDFS-1508


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
 1043158 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
 1043158 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
 1043158 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1043158 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1043158 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java
 1043158 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/UpgradeUtilities.java
 1043158 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogRace.java
 1043158 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestParallelImageWrite.java
 1043158 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestSaveNamespace.java
 1043158 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
 1043158 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/tools/offlineImageViewer/TestOfflineImageViewer.java
 1043158 

Diff: https://reviews.apache.org/r/125/diff


Testing
---

Unit test attached


Thanks,

Dhruba

Re: Review Request: Ability to do savenamespace without being in safemode

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/125/
---

(Updated 2010-12-02 11:21:23.394822)


Review request for hadoop-hdfs.


Changes
---

This addresses Konstanitin's request to add a unit test to test the invocation 
of saveNamespace command in the middle of a checkpoint.


Summary
---

The namenode need not be in safemode while runnign the saveNamespace command. 
The saveNamespace command acquires the FSNamesystem writelock, thus preventing 
anybody else from modifying the namespace.

The lease expiry thread in the LeaseManager acquires the FSNamesystem-writelock 
too, so it is well protected.


This addresses bug HDFS-1508.
https://issues.apache.org/jira/browse/HDFS-1508


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
 1041540 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
 1041540 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
 1041540 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1041540 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1041540 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java
 1041540 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/UpgradeUtilities.java
 1041540 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogRace.java
 1041540 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestParallelImageWrite.java
 1041540 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestSaveNamespace.java
 1041540 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
 1041540 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/tools/offlineImageViewer/TestOfflineImageViewer.java
 1041540 

Diff: https://reviews.apache.org/r/125/diff


Testing
---

Unit test attached


Thanks,

Dhruba

Re: Review Request: Ability to do savenamespace without being in safemode

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/125/
---

(Updated 2010-12-01 23:05:00.973158)


Review request for hadoop-hdfs.


Changes
---

This patch introduces a "force" option to the saveNamespace command. If this 
option is set, then the saveNamespace command is executed even if the namenode 
is not in safemode. The ClientProtocol number if bumped up by one.


Summary
---

The namenode need not be in safemode while runnign the saveNamespace command. 
The saveNamespace command acquires the FSNamesystem writelock, thus preventing 
anybody else from modifying the namespace.

The lease expiry thread in the LeaseManager acquires the FSNamesystem-writelock 
too, so it is well protected.


This addresses bug HDFS-1508.
https://issues.apache.org/jira/browse/HDFS-1508


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/tools/DFSAdmin.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/UpgradeUtilities.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestEditLogRace.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestParallelImageWrite.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestSaveNamespace.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/tools/offlineImageViewer/TestOfflineImageViewer.java
 1040699 

Diff: https://reviews.apache.org/r/125/diff


Testing
---

Unit test attached


Thanks,

Dhruba

Review Request: Ability to do savenamespace without being in safemode

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/125/
---

Review request for hadoop-hdfs.


Summary
---

The namenode need not be in safemode while runnign the saveNamespace command. 
The saveNamespace command acquires the FSNamesystem writelock, thus preventing 
anybody else from modifying the namespace.

The lease expiry thread in the LeaseManager acquires the FSNamesystem-writelock 
too, so it is well protected.


This addresses bug HDFS-1508.
https://issues.apache.org/jira/browse/HDFS-1508


Diffs
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1040699 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestSaveNamespace.java
 1040699 

Diff: https://reviews.apache.org/r/125/diff


Testing
---

Unit test attached


Thanks,

Dhruba

Re: Review Request: Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18/
---

(Updated 2010-11-22 13:47:46.894388)


Review request for hadoop-hdfs.


Changes
---

Fixed javadoc warnings.


Summary
---

Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)

As discussed in HDFS-, it would be beneficial for tools such as the RAID 
block fixer and RAID FSCK to have access to listCorruptFileBlocks via the 
DistributedFileSystem (rather than having to parse Servlet output, which could 
present a performance problem).

For further details, see https://issues.apache.org/jira/browse/HDFS-1482


This addresses bug HDFS-1482.
https://issues.apache.org/jira/browse/HDFS-1482


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/fs/Hdfs.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCorruptFilesJsp.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFsck.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/webapps/hdfs/corrupt_files.jsp
 1036663 

Diff: https://reviews.apache.org/r/18/diff


Testing
---

Unit tests (including new test case in TestListCorruptFileBlocks)


Thanks,

Patrick

Re: Review Request: Populate needed replication queues before leaving safe mode.

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/105/
---

(Updated 2010-11-19 13:07:20.231197)


Review request for hadoop-hdfs.


Changes
---

Updated test case to play nice with HDFS-1482.


Summary
---

This patch introduces a new configuration variable 
dfs.namenode.replqueue.threshold-pct that determines the fraction of blocks for 
which block reports have to be received before the NameNode will start 
initializing the needed replication queues. Once a sufficient number of block 
reports have been received, the queues are initialized while the NameNode is 
still in safe mode. After the queues are initialized, subsequent block reports 
are handled by updating the queues incrementally.

The benefit of this is twofold:
- It allows us to compute the replication queues while we are waiting for the 
last few block reports (when the NameNode is mostly idle). Once these block 
reports have been received, we can then immediately leave safe mode without 
having to wait for the computation of the needed replication queues (which 
requires a full traversal of the blocks map).
- With Raid, it may not be necessary to stay in safe mode until all blocks have 
been reported. Using this change, we could monitor if all of the missing blocks 
can be recreated using parity information and if so leave safe mode early. In 
order for this monitoring to work, we need access to the needed replication 
queues while the NameNode is still in safe mode.


This addresses bug HDFS-1476.
https://issues.apache.org/jira/browse/HDFS-1476


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/MiniDFSCluster.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
 1035545 

Diff: https://reviews.apache.org/r/105/diff


Testing
---

new test case in TestListCorruptFileBlocks


Thanks,

Patrick

Re: Review Request: Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18/
---

(Updated 2010-11-18 20:10:36.970120)


Review request for hadoop-hdfs.


Changes
---

Added listCorruptFileBlocks to FileContext.


Summary
---

Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)

As discussed in HDFS-, it would be beneficial for tools such as the RAID 
block fixer and RAID FSCK to have access to listCorruptFileBlocks via the 
DistributedFileSystem (rather than having to parse Servlet output, which could 
present a performance problem).

For further details, see https://issues.apache.org/jira/browse/HDFS-1482


This addresses bug HDFS-1482.
https://issues.apache.org/jira/browse/HDFS-1482


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/fs/Hdfs.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCorruptFilesJsp.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFsck.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
 1036663 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/webapps/hdfs/corrupt_files.jsp
 1036663 

Diff: https://reviews.apache.org/r/18/diff


Testing
---

Unit tests (including new test case in TestListCorruptFileBlocks)


Thanks,

Patrick

Re: Review Request: Populate needed replication queues before leaving safe mode.

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/105/
---

(Updated 2010-11-18 10:49:38.102334)


Review request for hadoop-hdfs.


Changes
---

Changed default value of replication queue threshold to safe mode threshold.


Summary
---

This patch introduces a new configuration variable 
dfs.namenode.replqueue.threshold-pct that determines the fraction of blocks for 
which block reports have to be received before the NameNode will start 
initializing the needed replication queues. Once a sufficient number of block 
reports have been received, the queues are initialized while the NameNode is 
still in safe mode. After the queues are initialized, subsequent block reports 
are handled by updating the queues incrementally.

The benefit of this is twofold:
- It allows us to compute the replication queues while we are waiting for the 
last few block reports (when the NameNode is mostly idle). Once these block 
reports have been received, we can then immediately leave safe mode without 
having to wait for the computation of the needed replication queues (which 
requires a full traversal of the blocks map).
- With Raid, it may not be necessary to stay in safe mode until all blocks have 
been reported. Using this change, we could monitor if all of the missing blocks 
can be recreated using parity information and if so leave safe mode early. In 
order for this monitoring to work, we need access to the needed replication 
queues while the NameNode is still in safe mode.


This addresses bug HDFS-1476.
https://issues.apache.org/jira/browse/HDFS-1476


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/MiniDFSCluster.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
 1035545 

Diff: https://reviews.apache.org/r/105/diff


Testing
---

new test case in TestListCorruptFileBlocks


Thanks,

Patrick

Re: Review Request: Populate needed replication queues before leaving safe mode.

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/105/
---

(Updated 2010-11-16 18:01:44.268029)


Review request for hadoop-hdfs.


Changes
---

Incorporated Dhruba's feedback. Thank you!


Summary
---

This patch introduces a new configuration variable 
dfs.namenode.replqueue.threshold-pct that determines the fraction of blocks for 
which block reports have to be received before the NameNode will start 
initializing the needed replication queues. Once a sufficient number of block 
reports have been received, the queues are initialized while the NameNode is 
still in safe mode. After the queues are initialized, subsequent block reports 
are handled by updating the queues incrementally.

The benefit of this is twofold:
- It allows us to compute the replication queues while we are waiting for the 
last few block reports (when the NameNode is mostly idle). Once these block 
reports have been received, we can then immediately leave safe mode without 
having to wait for the computation of the needed replication queues (which 
requires a full traversal of the blocks map).
- With Raid, it may not be necessary to stay in safe mode until all blocks have 
been reported. Using this change, we could monitor if all of the missing blocks 
can be recreated using parity information and if so leave safe mode early. In 
order for this monitoring to work, we need access to the needed replication 
queues while the NameNode is still in safe mode.


This addresses bug HDFS-1476.
https://issues.apache.org/jira/browse/HDFS-1476


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/MiniDFSCluster.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
 1035545 

Diff: https://reviews.apache.org/r/105/diff


Testing
---

new test case in TestListCorruptFileBlocks


Thanks,

Patrick

Re: Review Request: Populate needed replication queues before leaving safe mode.

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/105/#review39
---



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
<https://reviews.apache.org/r/105/#comment27>

Please change the default to 1, so that it is backward compatible.



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
<https://reviews.apache.org/r/105/#comment29>

We can first check canInitializeReplQueue to optimize on CPU.



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
<https://reviews.apache.org/r/105/#comment28>

This can move to after the SafeMode daemon is created.


- Dhruba


On 2010-11-16 16:39:18, Patrick Kling wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/105/
> ---
> 
> (Updated 2010-11-16 16:39:18)
> 
> 
> Review request for hadoop-hdfs.
> 
> 
> Summary
> ---
> 
> This patch introduces a new configuration variable 
> dfs.namenode.replqueue.threshold-pct that determines the fraction of blocks 
> for which block reports have to be received before the NameNode will start 
> initializing the needed replication queues. Once a sufficient number of block 
> reports have been received, the queues are initialized while the NameNode is 
> still in safe mode. After the queues are initialized, subsequent block 
> reports are handled by updating the queues incrementally.
> 
> The benefit of this is twofold:
> - It allows us to compute the replication queues while we are waiting for the 
> last few block reports (when the NameNode is mostly idle). Once these block 
> reports have been received, we can then immediately leave safe mode without 
> having to wait for the computation of the needed replication queues (which 
> requires a full traversal of the blocks map).
> - With Raid, it may not be necessary to stay in safe mode until all blocks 
> have been reported. Using this change, we could monitor if all of the missing 
> blocks can be recreated using parity information and if so leave safe mode 
> early. In order for this monitoring to work, we need access to the needed 
> replication queues while the NameNode is still in safe mode.
> 
> 
> This addresses bug HDFS-1476.
> https://issues.apache.org/jira/browse/HDFS-1476
> 
> 
> Diffs
> -
> 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
>  1035545 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
>  1035545 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
>  1035545 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/MiniDFSCluster.java
>  1035545 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
>  1035545 
> 
> Diff: https://reviews.apache.org/r/105/diff
> 
> 
> Testing
> ---
> 
> new test case in TestListCorruptFileBlocks
> 
> 
> Thanks,
> 
> Patrick
> 
>

Review Request: Populate needed replication queues before leaving safe mode.

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/105/
---

Review request for hadoop-hdfs.


Summary
---

This patch introduces a new configuration variable 
dfs.namenode.replqueue.threshold-pct that determines the fraction of blocks for 
which block reports have to be received before the NameNode will start 
initializing the needed replication queues. Once a sufficient number of block 
reports have been received, the queues are initialized while the NameNode is 
still in safe mode. After the queues are initialized, subsequent block reports 
are handled by updating the queues incrementally.

The benefit of this is twofold:
- It allows us to compute the replication queues while we are waiting for the 
last few block reports (when the NameNode is mostly idle). Once these block 
reports have been received, we can then immediately leave safe mode without 
having to wait for the computation of the needed replication queues (which 
requires a full traversal of the blocks map).
- With Raid, it may not be necessary to stay in safe mode until all blocks have 
been reported. Using this change, we could monitor if all of the missing blocks 
can be recreated using parity information and if so leave safe mode early. In 
order for this monitoring to work, we need access to the needed replication 
queues while the NameNode is still in safe mode.


This addresses bug HDFS-1476.
https://issues.apache.org/jira/browse/HDFS-1476


Diffs
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSConfigKeys.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/BlockManager.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/MiniDFSCluster.java
 1035545 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
 1035545 

Diff: https://reviews.apache.org/r/105/diff


Testing
---

new test case in TestListCorruptFileBlocks


Thanks,

Patrick

Re: Review Request: Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18/
---

(Updated 2010-11-08 19:01:36.459005)


Review request for hadoop-hdfs.


Changes
---

Added listCorruptFileBlocks to FileSystem


Summary
---

Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)

As discussed in HDFS-, it would be beneficial for tools such as the RAID 
block fixer and RAID FSCK to have access to listCorruptFileBlocks via the 
DistributedFileSystem (rather than having to parse Servlet output, which could 
present a performance problem).

For further details, see https://issues.apache.org/jira/browse/HDFS-1482


This addresses bug HDFS-1482.
https://issues.apache.org/jira/browse/HDFS-1482


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
 1032664 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
 1032664 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/HftpFileSystem.java
 1032664 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
 1032664 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1032664 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java
 1032664 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCorruptFilesJsp.java
 1032664 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFsck.java
 1032664 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
 1032664 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/webapps/hdfs/corrupt_files.jsp
 1032664 

Diff: https://reviews.apache.org/r/18/diff


Testing
---

Unit tests (including new test case in TestListCorruptFileBlocks)


Thanks,

Patrick

Re: Review Request: DFSClient.getBlockLocations returns BlockLocations with no indication that the corresponding blocks are corrupt

[Todd Lipcon]

Hi Arun,

I filed https://issues.apache.org/jira/browse/INFRA-3153

Thanks
-Todd

On Wed, Nov 3, 2010 at 9:47 PM, Arun C Murthy  wrote:

>
> On Nov 3, 2010, at 5:03 PM, Todd Lipcon wrote:
>
>  I wrote such a procmail script for review.hbase.org and posted it for the
>> ASF Infra guys a few weeks ago. We can file a new INFRA JIRA to get them
>> to
>> install/configure it.
>>
>>
> +1, thanks Todd!
>
>


-- 
Todd Lipcon
Software Engineer, Cloudera

Re: Review Request: DFSClient.getBlockLocations returns BlockLocations with no indication that the corresponding blocks are corrupt

[Arun C Murthy]

On Nov 3, 2010, at 5:03 PM, Todd Lipcon wrote:

I wrote such a procmail script for review.hbase.org and posted it  
for the
ASF Infra guys a few weeks ago. We can file a new INFRA JIRA to get  
them to

install/configure it.



+1, thanks Todd!

Re: Review Request: DFSClient.getBlockLocations returns BlockLocations with no indication that the corresponding blocks are corrupt

[Todd Lipcon]

I wrote such a procmail script for review.hbase.org and posted it for the
ASF Infra guys a few weeks ago. We can file a new INFRA JIRA to get them to
install/configure it.

-Todd

On Wed, Nov 3, 2010 at 1:27 PM, Arun C Murthy  wrote:

> Can we get RB to send these to jira? Having comments here and on jira is
> very confusing...
>
>
> On Nov 3, 2010, at 11:11 AM, Ramkumar Vadali wrote:
>
>
>> ---
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/27/#review27
>> ---
>>
>>
>>
>>
>> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSUtil.java
>> <https://reviews.apache.org/r/27/#comment17>
>>
>>   It should be sufficient to pass blk.isCorrupt() here. The client can
>> check the number of locations based on the remaining information.
>>
>>
>> - Ramkumar
>>
>>
>> On 2010-11-02 21:12:28, Patrick Kling wrote:
>>
>>>
>>> ---
>>> This is an automatically generated e-mail. To reply, visit:
>>> https://reviews.apache.org/r/27/
>>> ---
>>>
>>> (Updated 2010-11-02 21:12:28)
>>>
>>>
>>> Review request for hadoop-hdfs.
>>>
>>>
>>> Summary
>>> ---
>>>
>>> DFSClient.getBlockLocations returns BlockLocations with no indication
>>> that the corresponding blocks are corrupt
>>>
>>> When there are no uncorrupted replicas of a block,
>>> FSNamesystem.getBlockLocations returns LocatedBlocks corresponding to
>>> corrupt blocks. When DFSClient converts these to BlockLocations, the
>>> information that the corresponding block is corrupt is lost. We should add a
>>> field to BlockLocation to indicate whether the corresponding block is
>>> corrupt in order to warn the client that reading this block will fail. This
>>> would be especially useful for tools such as RAID FSCK, which could then
>>> easily inspect whether data or parity blocks are corrupted without having to
>>> make direct RPC calls
>>>
>>>
>>> This addresses bug HDFS-1483.
>>>   https://issues.apache.org/jira/browse/HDFS-1483
>>>
>>>
>>> Diffs
>>> -
>>>
>>>
>>> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSUtil.java1028386
>>>
>>> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/TestDFSUtil.javaPRE-CREATION
>>>
>>> Diff: https://reviews.apache.org/r/27/diff
>>>
>>>
>>> Testing
>>> ---
>>>
>>> TestDFSUtil
>>>
>>>
>>> Thanks,
>>>
>>> Patrick
>>>
>>>
>>>
>>
>


-- 
Todd Lipcon
Software Engineer, Cloudera

Re: Review Request: DFSClient.getBlockLocations returns BlockLocations with no indication that the corresponding blocks are corrupt

[Arun C Murthy]

Can we get RB to send these to jira? Having comments here and on jira  
is very confusing...


On Nov 3, 2010, at 11:11 AM, Ramkumar Vadali wrote:



---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27/#review27
---



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSUtil.java
<https://reviews.apache.org/r/27/#comment17>

   It should be sufficient to pass blk.isCorrupt() here. The client  
can check the number of locations based on the remaining information.



- Ramkumar


On 2010-11-02 21:12:28, Patrick Kling wrote:


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27/
---

(Updated 2010-11-02 21:12:28)


Review request for hadoop-hdfs.


Summary
---

DFSClient.getBlockLocations returns BlockLocations with no  
indication that the corresponding blocks are corrupt


When there are no uncorrupted replicas of a block,  
FSNamesystem.getBlockLocations returns LocatedBlocks corresponding  
to corrupt blocks. When DFSClient converts these to BlockLocations,  
the information that the corresponding block is corrupt is lost. We  
should add a field to BlockLocation to indicate whether the  
corresponding block is corrupt in order to warn the client that  
reading this block will fail. This would be especially useful for  
tools such as RAID FSCK, which could then easily inspect whether  
data or parity blocks are corrupted without having to make direct  
RPC calls



This addresses bug HDFS-1483.
   https://issues.apache.org/jira/browse/HDFS-1483


Diffs
-

 http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSUtil.java 
 1028386
 http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/TestDFSUtil.java 
 PRE-CREATION


Diff: https://reviews.apache.org/r/27/diff


Testing
---

TestDFSUtil


Thanks,

Patrick

Re: Review Request: DFSClient.getBlockLocations returns BlockLocations with no indication that the corresponding blocks are corrupt

[Patrick Kling]

> On 2010-11-03 11:41:39, Ramkumar Vadali wrote:
> > Looks good to me, but this diff depends on a hadoop-common change, right?

It depends on HADOOP-7013, which can be found here: 
https://reviews.apache.org/r/26/


- Patrick


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27/#review29
---


On 2010-11-03 11:33:39, Patrick Kling wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27/
> ---
> 
> (Updated 2010-11-03 11:33:39)
> 
> 
> Review request for hadoop-hdfs.
> 
> 
> Summary
> ---
> 
> DFSClient.getBlockLocations returns BlockLocations with no indication that 
> the corresponding blocks are corrupt
> 
> When there are no uncorrupted replicas of a block, 
> FSNamesystem.getBlockLocations returns LocatedBlocks corresponding to corrupt 
> blocks. When DFSClient converts these to BlockLocations, the information that 
> the corresponding block is corrupt is lost. We should add a field to 
> BlockLocation to indicate whether the corresponding block is corrupt in order 
> to warn the client that reading this block will fail. This would be 
> especially useful for tools such as RAID FSCK, which could then easily 
> inspect whether data or parity blocks are corrupted without having to make 
> direct RPC calls
> 
> 
> This addresses bug HDFS-1483.
> https://issues.apache.org/jira/browse/HDFS-1483
> 
> 
> Diffs
> -
> 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSUtil.java
>  1028386 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/TestDFSUtil.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/27/diff
> 
> 
> Testing
> ---
> 
> TestDFSUtil
> 
> 
> Thanks,
> 
> Patrick
> 
>

Re: Review Request: DFSClient.getBlockLocations returns BlockLocations with no indication that the corresponding blocks are corrupt

[Ramkumar Vadali]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27/#review29
---


Looks good to me, but this diff depends on a hadoop-common change, right?

- Ramkumar


On 2010-11-03 11:33:39, Patrick Kling wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27/
> ---
> 
> (Updated 2010-11-03 11:33:39)
> 
> 
> Review request for hadoop-hdfs.
> 
> 
> Summary
> ---
> 
> DFSClient.getBlockLocations returns BlockLocations with no indication that 
> the corresponding blocks are corrupt
> 
> When there are no uncorrupted replicas of a block, 
> FSNamesystem.getBlockLocations returns LocatedBlocks corresponding to corrupt 
> blocks. When DFSClient converts these to BlockLocations, the information that 
> the corresponding block is corrupt is lost. We should add a field to 
> BlockLocation to indicate whether the corresponding block is corrupt in order 
> to warn the client that reading this block will fail. This would be 
> especially useful for tools such as RAID FSCK, which could then easily 
> inspect whether data or parity blocks are corrupted without having to make 
> direct RPC calls
> 
> 
> This addresses bug HDFS-1483.
> https://issues.apache.org/jira/browse/HDFS-1483
> 
> 
> Diffs
> -
> 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSUtil.java
>  1028386 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/TestDFSUtil.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/27/diff
> 
> 
> Testing
> ---
> 
> TestDFSUtil
> 
> 
> Thanks,
> 
> Patrick
> 
>

Re: Review Request: DFSClient.getBlockLocations returns BlockLocations with no indication that the corresponding blocks are corrupt

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27/
---

(Updated 2010-11-03 11:33:39.415750)


Review request for hadoop-hdfs.


Changes
---

Incorporated Ram's feedback. Thank you!


Summary
---

DFSClient.getBlockLocations returns BlockLocations with no indication that the 
corresponding blocks are corrupt

When there are no uncorrupted replicas of a block, 
FSNamesystem.getBlockLocations returns LocatedBlocks corresponding to corrupt 
blocks. When DFSClient converts these to BlockLocations, the information that 
the corresponding block is corrupt is lost. We should add a field to 
BlockLocation to indicate whether the corresponding block is corrupt in order 
to warn the client that reading this block will fail. This would be especially 
useful for tools such as RAID FSCK, which could then easily inspect whether 
data or parity blocks are corrupted without having to make direct RPC calls


This addresses bug HDFS-1483.
https://issues.apache.org/jira/browse/HDFS-1483


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSUtil.java
 1028386 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/TestDFSUtil.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/27/diff


Testing
---

TestDFSUtil


Thanks,

Patrick

Re: Review Request: DFSClient.getBlockLocations returns BlockLocations with no indication that the corresponding blocks are corrupt

[Ramkumar Vadali]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27/#review27
---



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSUtil.java
<https://reviews.apache.org/r/27/#comment17>

It should be sufficient to pass blk.isCorrupt() here. The client can check 
the number of locations based on the remaining information.


- Ramkumar


On 2010-11-02 21:12:28, Patrick Kling wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27/
> ---
> 
> (Updated 2010-11-02 21:12:28)
> 
> 
> Review request for hadoop-hdfs.
> 
> 
> Summary
> ---
> 
> DFSClient.getBlockLocations returns BlockLocations with no indication that 
> the corresponding blocks are corrupt
> 
> When there are no uncorrupted replicas of a block, 
> FSNamesystem.getBlockLocations returns LocatedBlocks corresponding to corrupt 
> blocks. When DFSClient converts these to BlockLocations, the information that 
> the corresponding block is corrupt is lost. We should add a field to 
> BlockLocation to indicate whether the corresponding block is corrupt in order 
> to warn the client that reading this block will fail. This would be 
> especially useful for tools such as RAID FSCK, which could then easily 
> inspect whether data or parity blocks are corrupted without having to make 
> direct RPC calls
> 
> 
> This addresses bug HDFS-1483.
> https://issues.apache.org/jira/browse/HDFS-1483
> 
> 
> Diffs
> -
> 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSUtil.java
>  1028386 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/TestDFSUtil.java
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/27/diff
> 
> 
> Testing
> ---
> 
> TestDFSUtil
> 
> 
> Thanks,
> 
> Patrick
> 
>

Review Request: DFSClient.getBlockLocations returns BlockLocations with no indication that the corresponding blocks are corrupt

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27/
---

Review request for hadoop-hdfs.


Summary
---

DFSClient.getBlockLocations returns BlockLocations with no indication that the 
corresponding blocks are corrupt

When there are no uncorrupted replicas of a block, 
FSNamesystem.getBlockLocations returns LocatedBlocks corresponding to corrupt 
blocks. When DFSClient converts these to BlockLocations, the information that 
the corresponding block is corrupt is lost. We should add a field to 
BlockLocation to indicate whether the corresponding block is corrupt in order 
to warn the client that reading this block will fail. This would be especially 
useful for tools such as RAID FSCK, which could then easily inspect whether 
data or parity blocks are corrupted without having to make direct RPC calls


This addresses bug HDFS-1483.
https://issues.apache.org/jira/browse/HDFS-1483


Diffs
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSUtil.java
 1028386 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/TestDFSUtil.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/27/diff


Testing
---

TestDFSUtil


Thanks,

Patrick

Re: Review Request: Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18/
---

(Updated 2010-11-01 11:50:37.134842)


Review request for hadoop-hdfs.


Changes
---

ClientProtocol.listCorruptFileBlocks now returns a list of file names and a 
cookie string, which can be used to iteratively retrieve all corrupt files.


Summary
---

Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)

As discussed in HDFS-, it would be beneficial for tools such as the RAID 
block fixer and RAID FSCK to have access to listCorruptFileBlocks via the 
DistributedFileSystem (rather than having to parse Servlet output, which could 
present a performance problem).

For further details, see https://issues.apache.org/jira/browse/HDFS-1482


This addresses bug HDFS-1482.
https://issues.apache.org/jira/browse/HDFS-1482


Diffs (updated)
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/CorruptFileBlocks.java
 PRE-CREATION 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCorruptFilesJsp.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFsck.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/webapps/hdfs/corrupt_files.jsp
 1028517 

Diff: https://reviews.apache.org/r/18/diff


Testing
---

Unit tests (including new test case in TestListCorruptFileBlocks)


Thanks,

Patrick

Re: Review Request: Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18/#review21
---



http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/CorruptFileBlock.java
<https://reviews.apache.org/r/18/#comment10>

I think we should not expose any block-related information via this API. It 
should return a bunch of filesnames that are corrupted along with a cookie that 
can be used for the next iterative call to this API.


- Dhruba


On 2010-10-29 09:50:27, Patrick Kling wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18/
> ---
> 
> (Updated 2010-10-29 09:50:27)
> 
> 
> Review request for hadoop-hdfs.
> 
> 
> Summary
> ---
> 
> Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)
> 
> As discussed in HDFS-, it would be beneficial for tools such as the RAID 
> block fixer and RAID FSCK to have access to listCorruptFileBlocks via the 
> DistributedFileSystem (rather than having to parse Servlet output, which 
> could present a performance problem).
> 
> For further details, see https://issues.apache.org/jira/browse/HDFS-1482
> 
> 
> This addresses bug HDFS-1482.
> https://issues.apache.org/jira/browse/HDFS-1482
> 
> 
> Diffs
> -
> 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
>  1028517 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
>  1028517 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
>  1028517 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/CorruptFileBlock.java
>  PRE-CREATION 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
>  1028517 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java
>  1028517 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCorruptFilesJsp.java
>  1028517 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFsck.java
>  1028517 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
>  1028517 
>   
> http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/webapps/hdfs/corrupt_files.jsp
>  1028517 
> 
> Diff: https://reviews.apache.org/r/18/diff
> 
> 
> Testing
> ---
> 
> Unit tests (including new test case in TestListCorruptFileBlocks)
> 
> 
> Thanks,
> 
> Patrick
> 
>

Review Request: Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)

[Patrick Kling]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18/
---

Review request for hadoop-hdfs.


Summary
---

Add listCorruptFileBlocks to DistributedFileSystem (and ClientProtocol)

As discussed in HDFS-, it would be beneficial for tools such as the RAID 
block fixer and RAID FSCK to have access to listCorruptFileBlocks via the 
DistributedFileSystem (rather than having to parse Servlet output, which could 
present a performance problem).

For further details, see https://issues.apache.org/jira/browse/HDFS-1482


This addresses bug HDFS-1482.
https://issues.apache.org/jira/browse/HDFS-1482


Diffs
-

  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DFSClient.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/DistributedFileSystem.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/ClientProtocol.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/protocol/CorruptFileBlock.java
 PRE-CREATION 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NameNode.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/java/org/apache/hadoop/hdfs/server/namenode/NamenodeFsck.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestCorruptFilesJsp.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestFsck.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/test/hdfs/org/apache/hadoop/hdfs/server/namenode/TestListCorruptFileBlocks.java
 1028517 
  
http://svn.apache.org/repos/asf/hadoop/hdfs/trunk/src/webapps/hdfs/corrupt_files.jsp
 1028517 

Diff: https://reviews.apache.org/r/18/diff


Testing
---

Unit tests (including new test case in TestListCorruptFileBlocks)


Thanks,

Patrick

Re: Review Request: Test review

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13/
---

(Updated 2010-10-26 17:27:35.304640)


Review request for hadoop-hdfs and Dmytro Molkov.


Summary
---

Test Review request


This addresses bug HDFS-222.
https://issues.apache.org/jira/browse/HDFS-222


Diffs
-


Diff: https://reviews.apache.org/r/13/diff


Testing
---

Test review


Thanks,

Dhruba

Review Request: Test review

[Dhruba Borthakur]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/13/
---

Review request for hadoop-hdfs and Dmytro Molkov.


Summary
---

Test Review request


Diffs
-


Diff: https://reviews.apache.org/r/13/diff


Testing
---

Test review


Thanks,

Dhruba