[#HADOOP-2345] new transactions to support HDFS Appends

Hadoop Common

new transactions to support HDFS Appends

Created: 04/Dec/07 06:19 PM Updated: 08/Jul/09 04:42 PM

Component/s:

None

Affects Version/s:

None

Fix Version/s:

0.17.0

Time Tracking:

Not Specified

File Attachments:

File Name	Date Attached	Attached By	Size
AppendTransactions.patch	2008-02-03 09:43 AM	dhruba borthakur	45 kB
AppendTransactions2.patch	2008-02-07 12:37 AM	dhruba borthakur	46 kB
AppendTransactions3.patch	2008-02-11 08:07 AM	dhruba borthakur	46 kB
AppendTransactions4.patch	2008-02-12 12:50 AM	dhruba borthakur	48 kB
AppendTransactions5.patch	2008-02-13 10:47 PM	dhruba borthakur	48 kB

Issue Links:

Blocker

This issue blocks:

~~HADOOP-1700~~ Append to files in HDFS

Hadoop Flags:	Incompatible change
Release Note:	Introduce new namenode transactions to support appending to HDFS files.
Resolution Date:	14/Feb/08 05:37 AM

Description

« Hide

This JIRA adresses the changes needed to the transaction mechanism to support appending data to existing HDFS files. The details of this design is documented in ~~HADOOP-1700~~.

All

Comments

Work Log

Change History

Subversion Commits

Sort Order:

[ Permlink | « Hide ]

dhruba borthakur added a comment - 03/Feb/08 09:43 AM

This patch implements the following two functionality:

1. A client can continue to write to a DFSOutputStream even if the namenode restarts! This patch persist leases in the transaction log. Persisting leases is a pre-requisite for correct error recovery for the HDFS file-append feature.

2. A new method NameNode.fsync() that lets a client persist metadata for a file into transaction log.

[ Permlink | « Hide ]

dhruba borthakur added a comment - 07/Feb/08 12:37 AM

Changed names of Transactions from Openlease/CloseLease to Open/Close. Removed DatanodeDescriptors from fsimage.

[ Permlink | « Hide ]

Konstantin Shvachko added a comment - 08/Feb/08 10:48 PM - edited

ClientProtocol.versionID should be incremented.
NameNode.fsync() should use @inheritDoc.
FSDirectory.delete(String src, INode[] old) needs documentation on the parameter "old".
Since you return only one INode I'd change prototype of unprotectedDelete
```
Block[] unprotectedDelete(String src, long modificationTime, INode[] old)
```
to
```
INode unprotectedDelete(String src, long modificationTime, Collection<Block> deletedBlocks)
```
Replacing Block[] to Collection<Block> will probably be required, since we do not know in
advance how many blocks will be returned. It's a good thing anyway, since collections
are more flexible than arrays.
getFileINode() I am not sure about the consequences of removing waitForReady() from it.
I understand you need to look for files under construction before the entire image is loaded,
but doesn't it break behavior of other methods. E.g. one will be able to call
getBlockLocations() or startFile() before the image is loaded.
OP_ADD sounds good to me. It can mean adding a file or adding a block.
```
OP_OPEN = 9;    // create for write
```
sounds controversial, doesn't it?
So may be you should retain OP_ADD in favor replacing it with OP_OPEN.
OP_GENSTAMP: I'd propose OP_SET_GENERATION instead.
You use different comparisons, which are equivalent but confusing in detecting the version they refer to.
```
logVersion <= -12
logVersion < -11
```
I'd just use logVersion <= -12 everywhere.
Why generation stamp needs to be serialized as a one element array?
Arrays are appropriate if several members need to be serialized at once.
In the case you can simply in.readLong() / writeLong().
The same as in FSImage.

Do we want to introduce something like

ClientID {
  protected StringBytesWritable clientName;         // lease holder
  protected StringBytesWritable clientMachine;
}

Is there a use case for this?

FSImage.loadFSImage(File)
You want to "read in the last generation stamp" after imgVersion and numFile are read and sorted out.
Somewhere after
```
this.layoutVersion = imgVersion;
```
Otherwise loading of pre-version images will fail. It's unlikely somebody will need that, but still.
FSNamesystem.generationStamp: Could you please use JavaDoc-style comments for the description of this member.
FSDirectory.closeLease() is confusing because it has nothing to do with leases.
I was first wondering why it is a part of FSDirectory while other lease methods are in FSNamesystem,
but then realized that it is just closing the file. Please rename, and also the comments to the calls
of closeLease() say "persist block allocations for this file" which does not reflect the actual action
any more.
GenerationStamp may implement WritableComparable rather than Writable, Comparable

I like that serialization code for INodeUnderConstruction is separated from the loadFSImage mainstream
code. I am not so sure it should implement Writable because this class is not supposed to be a part of
any communications. Alternatively we could define methods

FSImage.readINodeUnderConstruction(in, INodeUnderConstruction)
FSImage.writeINodeUnderConstruction(out, INodeUnderConstruction)

static INodeUnderConstruction.readINode(in, INodeUnderConstruction)
static INodeUnderConstruction.writeINode(out, INodeUnderConstruction)

[ Permlink | « Hide ]

dhruba borthakur added a comment - 11/Feb/08 08:07 AM

Incorporated all of Konstantin's comments except 10 and 15.
I like the serialization code in INodeUnderConstruction, especially because it writes the serialized information to the transaction log.

[ Permlink | « Hide ]

dhruba borthakur added a comment - 11/Feb/08 08:25 AM

Submitting for HadoopQA tests (pending final round of review from Konstantin).

[ Permlink | « Hide ]

Hadoop QA added a comment - 11/Feb/08 09:40 AM

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12375208/AppendTransactions3.patch
against trunk revision 619744.

@author +1. The patch does not contain any @author tags.

tests included +1. The patch appears to include 6 new or modified tests.

javadoc +1. The javadoc tool did not generate any warning messages.

javac -1. The applied patch generated 633 javac compiler warnings (more than the trunk's current 612 warnings).

release audit +1. The applied patch does not generate any new release audit warnings.

findbugs -1. The patch appears to introduce 1 new Findbugs warnings.

core tests +1. The patch passed core unit tests.

contrib tests +1. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1769/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1769/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1769/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1769/console

This message is automatically generated.

[ Permlink | « Hide ]

dhruba borthakur added a comment - 12/Feb/08 12:50 AM

Incorporated #15 from Konstantin's list of review comments.

[ Permlink | « Hide ]

dhruba borthakur added a comment - 12/Feb/08 12:51 AM

This patch will have a few javac compiler warnings because it uses UTF8 to serialize leases to the transaction log.

[ Permlink | « Hide ]

Hadoop QA added a comment - 12/Feb/08 04:09 AM

-1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12375292/AppendTransactions4.patch
against trunk revision 619744.

@author +1. The patch does not contain any @author tags.

tests included +1. The patch appears to include 6 new or modified tests.

javadoc +1. The javadoc tool did not generate any warning messages.

javac -1. The applied patch generated 629 javac compiler warnings (more than the trunk's current 608 warnings).

release audit +1. The applied patch does not generate any new release audit warnings.

findbugs +1. The patch does not introduce any new Findbugs warnings.

core tests +1. The patch passed core unit tests.

contrib tests +1. The patch passed contrib unit tests.

Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1779/testReport/
Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1779/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1779/artifact/trunk/build/test/checkstyle-errors.html
Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/1779/console

This message is automatically generated.

[ Permlink | « Hide ]

Konstantin Shvachko added a comment - 13/Feb/08 10:19 PM

FSEditLog.loadFSEdits(). This is what meant in (9):

case OP_SET_GENSTAMP: {
  long generation = in.readLong();
  fsDir.namesystem.setGenerationStamp(generation);

FSNamesystem.generationStamp. In (12) I meant

/** 
 * The global generation stamp for this file system. Valid values start from 1000.
 */
private GenerationStamp generationStamp = new GenerationStamp(1000);

I like the new prototype of FSDirectory.delete().
I suggest we simplify the loop statement in FSImage.deleteInternal()

INode old = dir.delete(src, deletedBlocks);
if (old == null)
  return false;
for (Block b : deletedBlocks) {
  .........
}

[ Permlink | « Hide ]

dhruba borthakur added a comment - 13/Feb/08 10:47 PM

FSEditLog.OP_SET_GENERATIONSTAMP has to use writables because the transactions are written using FSEditLog.logEdit(). This method allows writables to be written to the edit log.

I made changes (2) and (3) described above. Thanks Konstantin for your review comments.