Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,12 @@ public int getCapacity() {
/** @return true if there is no datanode storage associated with the block */
abstract boolean hasNoStorage();

/**
* Checks whether this block has a Provided replica.
* @return true if this block has a replica on Provided storage.
*/
abstract boolean isProvided();

/**
* Find specified DatanodeStorageInfo.
* @return DatanodeStorageInfo or null if not found.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import com.google.common.base.Preconditions;
import org.apache.hadoop.classification.InterfaceAudience;
import org.apache.hadoop.fs.StorageType;
import org.apache.hadoop.hdfs.protocol.Block;
import org.apache.hadoop.hdfs.protocol.BlockType;

Expand All @@ -28,12 +29,16 @@
@InterfaceAudience.Private
public class BlockInfoContiguous extends BlockInfo {

private boolean hasProvidedStorage;

public BlockInfoContiguous(short size) {
super(size);
hasProvidedStorage = false;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just set by default as false and not do it here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed it for now.

}

public BlockInfoContiguous(Block blk, short size) {
super(blk, size);
hasProvidedStorage = false;
}

/**
Expand Down Expand Up @@ -62,6 +67,9 @@ boolean addStorage(DatanodeStorageInfo storage, Block reportedBlock) {
// find the last null node
int lastNode = ensureCapacity(1);
setStorageInfo(lastNode, storage);
if (storage.getStorageType() == StorageType.PROVIDED) {
hasProvidedStorage = true;
}
return true;
}

Expand All @@ -77,9 +85,31 @@ boolean removeStorage(DatanodeStorageInfo storage) {
setStorageInfo(dnIndex, getStorageInfo(lastNode));
// set the last entry to null
setStorageInfo(lastNode, null);
if (storage.getStorageType() == StorageType.PROVIDED
&& !hasProvidedStorages()) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this already guarantee is false?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see is the other method which goes over each DN, nevermind.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you go over the overhead added to the regular pipeline without Provided?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the check and there is no invocation in the regular pipeline now.

hasProvidedStorage = false;
}
return true;
}

@Override
boolean isProvided() {
return hasProvidedStorage;
}

private boolean hasProvidedStorages() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

High level javadoc.

int len = getCapacity();
for(int idx = 0; idx < len; idx++) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

space after the for

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

DatanodeStorageInfo cur = getStorageInfo(idx);
if(cur != null) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space after the if

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

if (cur.getStorageType() == StorageType.PROVIDED) {
return true;
}
}
}
return false;
}

@Override
public int numNodes() {
assert this.storages != null : "BlockInfo is not initialized";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,11 @@ final boolean hasNoStorage() {
return true;
}

@Override
boolean isProvided() {
return false;
Comment thread
goiri marked this conversation as resolved.
}

/**
* This class contains datanode storage information and block index in the
* block group.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@

import static org.apache.hadoop.hdfs.server.namenode.INodeId.INVALID_INODE_ID;
import static org.hamcrest.core.Is.is;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import org.apache.hadoop.fs.StorageType;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.apache.hadoop.hdfs.DFSTestUtil;
Expand Down Expand Up @@ -64,6 +67,28 @@ public void testAddStorage() throws Exception {
Assert.assertEquals(storage, blockInfo.getStorageInfo(0));
}

@Test
public void testAddProvidedStorage() throws Exception {
BlockInfo blockInfo = new BlockInfoContiguous((short) 3);

DatanodeStorageInfo storage = mock(DatanodeStorageInfo.class);
when(storage.getStorageType()).thenReturn(StorageType.PROVIDED);
boolean added = blockInfo.addStorage(storage, blockInfo);

Assert.assertTrue(added);
Assert.assertEquals(storage, blockInfo.getStorageInfo(0));
Assert.assertTrue(blockInfo.isProvided());

Comment thread
goiri marked this conversation as resolved.
blockInfo = new BlockInfoContiguous((short) 3);
storage = mock(DatanodeStorageInfo.class);
when(storage.getStorageType()).thenReturn(StorageType.DISK);
added = blockInfo.addStorage(storage, blockInfo);

Assert.assertTrue(added);
Assert.assertEquals(storage, blockInfo.getStorageInfo(0));
Assert.assertFalse(blockInfo.isProvided());
}

@Test
public void testReplaceStorage() throws Exception {

Expand Down