[
https://issues.apache.org/jira/browse/HDFS-13578?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16490911#comment-16490911
]
Erik Krogen commented on HDFS-13578:
------------------------------------
Thanks for the comments [~elgoiri]! I agree about adding the atime marker -
it's a good reminder and doesn't hurt anything.
[~csun], patch is looking great, but should the following methods also have
{{@ReadOnly}} annotation?
{code}
/**
* Get an array of aggregated statistics combining blocks of both type
* {@link BlockType#CONTIGUOUS} and {@link BlockType#STRIPED} in the
* filesystem. Use public constants like {@link #GET_STATS_CAPACITY_IDX} in
* place of actual numbers to index into the array.
* <ul>
* <li> [0] contains the total storage capacity of the system, in bytes.</li>
* <li> [1] contains the total used space of the system, in bytes.</li>
* <li> [2] contains the available storage of the system, in bytes.</li>
* <li> [3] contains number of low redundancy blocks in the system.</li>
* <li> [4] contains number of corrupt blocks. </li>
* <li> [5] contains number of blocks without any good replicas left. </li>
* <li> [6] contains number of blocks which have replication factor
* 1 and have lost the only replica. </li>
* <li> [7] contains number of bytes that are at risk for deletion. </li>
* <li> [8] contains number of pending deletion blocks. </li>
* </ul>
*/
@Idempotent
long[] getStats() throws IOException;
/**
* Get statistics pertaining to blocks of type {@link BlockType#CONTIGUOUS}
* in the filesystem.
*/
@Idempotent
ReplicatedBlockStats getReplicatedBlockStats() throws IOException;
/**
* Get statistics pertaining to blocks of type {@link BlockType#STRIPED}
* in the filesystem.
*/
@Idempotent
ECBlockGroupStats getECBlockGroupStats() throws IOException;
/**
* Get a report on the system's current datanodes.
* One DatanodeInfo object is returned for each DataNode.
* Return live datanodes if type is LIVE; dead datanodes if type is DEAD;
* otherwise all datanodes if type is ALL.
*/
@Idempotent
DatanodeInfo[] getDatanodeReport(HdfsConstants.DatanodeReportType type)
throws IOException;
/**
* Get a report on the current datanode storages.
*/
@Idempotent
DatanodeStorageReport[] getDatanodeStorageReport(
HdfsConstants.DatanodeReportType type) throws IOException;
/**
* @return encryption key so a client can encrypt data sent via the
* DataTransferProtocol to/from DataNodes.
* @throws IOException
*/
@Idempotent
DataEncryptionKey getDataEncryptionKey() throws IOException;
@Idempotent
long getCurrentEditLogTxid() throws IOException;
/**
* Get an ordered list of batches of events corresponding to the edit log
* transactions for txids equal to or greater than txid.
*/
@Idempotent
EventBatchList getEditsFromTxid(long txid) throws IOException;
/**
* Get {@link QuotaUsage} rooted at the specified directory.
* @param path The string representation of the path
*
* @throws AccessControlException permission denied
* @throws java.io.FileNotFoundException file <code>path</code> is not found
* @throws org.apache.hadoop.fs.UnresolvedLinkException if <code>path</code>
* contains a symlink.
* @throws IOException If an I/O error occurred
*/
@Idempotent
QuotaUsage getQuotaUsage(String path) throws IOException;
{code}
{{getCurrentEditLogTxid()}} I am a little uncertain of. While it and the
corresponding {{getEditsFromTxid()}} are both read-only, we may want it to be
served from the active at some points, e.g. {{getCurrentEditLogTxid()}} can be
useful for implementing {{msync()}}. Probably we can mark it as {{@ReadOnly}}
and if it needs to go to the active there can be some extra logic.
Regarding the following two methods you've annotated with {{@ReadOnly}}, are
these safe to be read from standby?
{code}
/**
* List open files in the system in batches. INode id is the cursor and the
* open files returned in a batch will have their INode ids greater than
* the cursor INode id. Open files can only be requested by super user and
* the the list across batches are not atomic.
*
* @param prevId the cursor INode id.
* @throws IOException
*/
@Idempotent
@Deprecated
@ReadOnly
BatchedEntries<OpenFileEntry> listOpenFiles(long prevId) throws IOException;
/**
* List open files in the system in batches. INode id is the cursor and the
* open files returned in a batch will have their INode ids greater than
* the cursor INode id. Open files can only be requested by super user and
* the the list across batches are not atomic.
*
* @param prevId the cursor INode id.
* @param openFilesTypes types to filter the open files.
* @param path path to filter the open files.
* @throws IOException
*/
@Idempotent
@ReadOnly
BatchedEntries<OpenFileEntry> listOpenFiles(long prevId,
EnumSet<OpenFilesType> openFilesTypes, String path) throws IOException;
{code}
If a file is open for write, is the standby aware? I am a little worried that
if these go to the standby they will always return an empty set, but am not
sure.
I have a few minor style nits in {{ReadOnly.java}}:
* There shouldn't be line blank lines between the annotations and the class
definition.
* The Javadoc for {{atimeAffected}} has an extra blank line
* I think the Javadoc may go into too much detail about behavior. I
specifically don't like this sentence:
{quote}
if true, the target method may fail at invocation when updating last accessed
time is enabled on the NameNode side
{quote}
The failure semantics aren't really relevant to the annotation. I think it
should say something more along the lines of:
{code}
if true, the annotated method may update the last accessed time while
performing its read, if access time is enabled (via {@value
DFSConfigKeys.DFS_NAMENODE_ACCESSTIME_PRECISION_KEY})
{code}
> Add ReadOnly annotation to methods in ClientProtocol
> ----------------------------------------------------
>
> Key: HDFS-13578
> URL: https://issues.apache.org/jira/browse/HDFS-13578
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Reporter: Chao Sun
> Assignee: Chao Sun
> Priority: Major
> Attachments: HDFS-13578-HDFS-12943.000.patch,
> HDFS-13578-HDFS-12943.001.patch, HDFS-13578-HDFS-12943.002.patch,
> HDFS-13578-HDFS-12943.004.patch
>
>
> For those read-only methods in {{ClientProtocol}}, we may want to use a
> {{@ReadOnly}} annotation to mark them, and then check in the proxy provider
> for observer.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
