[
https://issues.apache.org/jira/browse/HDFS-12522?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16336723#comment-16336723
]
Anu Engineer commented on HDFS-12522:
-------------------------------------
[~xyao] and [~nandakumar131] thanks for the review comments. I have attached v2
patch that addresses all comments. The details are below:
{quote}Line:119 Instead of storing ContainerInfo in lastUsedMap we can just
store ContainerID.
{quote}
Fixed.
{quote}Line:310 Do we need handle Integer overflow case for containerCount?
{quote}
Fixed by adding a precondition check in setContainerID()
{quote}Line:313 If containers.createContainer(containerInfo) call fails, should
we revert containerCount?
{quote}
I have bypassed this problem completely, by making the container ID a 64 bit
value for time being. That means we don't have to worry about leaking a
container ID. We have enough of them.
{quote}Line:354 Javadoc to be updated stating that getMatchingContainer will
return null if there are no matching containers available.
{quote}
Fixed.
{quote}Line:363 containers.getMatchingContainerIDs can return null, so if
(matchingSet.size() == 0) has to be if (matchingSet == null ||
matchingSet.size() == 0)
{quote}
Fixed.
{quote}Line:369 We should store lastUsedContainer for each combination of
owner, type and factor instead of just owner
{quote}
Fixed.
{quote}Line:424 Can we rename getMatchingContainers to getMatchingContainerIDs
as it returns NavigableSet<ContainerID>?
{quote}
Fixed.
{{ContainerStateMap.java}}
{quote}Line:61 Typo Contianer -> Container
{quote}
Fixed.
{quote}Line:85 emptySet can be static.
{quote}
Fixed.
{quote}Line:115 rename createContainer to addContainer, as it does not create
any container but adds them to ContainerStateMap?
{quote}
{quote}Line:118, 162, 183, 234, 248, 262, 299 lock instance doesn't need to be
assigned to a variable.
{quote}
I know in Java 9, we don't need to do this, but my complier warns if I try to
remove this Java 8.
Line:141 Do we need getCurrentInfo(ContainerInfo info)? we can always use
getContainerInfo(int containerID).
Line:151 getContainerInfo can directly take ContainerID object instead of
creating new ContainerID object for each call (unnecessary object creation)
Both of these helper functions with appropriate uses. I have renamed
getCurentInfo to getContianerInfo.
{quote}Line:200 We should not use ContainerInfo that is passed as argument, it
will contain stale allocatedBytes value as it’s read from metadata db
(allocatedBytes value is updated in db only during SCM shutdown). We should
always do containerMap.get, update the state and then containerMap.put
{quote}
That does *not* work. Since we don't know what values of info is being updated.
If we want allocatedBytes to be corect, then it is has to updated at a level
higher than this.
updateState: We don’t need ContainerInfo as an argument, we can just have
containerID
Technically yes, but unfortunately, we use this to update all fields of the
ContainerInfo. That is when state change is executed, the user can change other
fields too.
{quote}Rename suggestions:
{quote}
Fixed.
{quote}Line:354 - 355 Random “+” & “*” in the javadoc.
{quote}
Fixed.
{quote}Line: 443 - 444 This can be removed.
{quote}
Line numbers are not correct?
{quote}Since we have removed PriorityQueue from ContainerStateManager, we don’t
need lastUsed time in ContainerInfo. We will be maintaining that information in
ContainerStateManager#lastUsedMap.
{quote}
We can also remove ContainerInfo#compare and ContainerInfo#compareTo methods.
I think this is still good metric to have, especially for debugging and testing
scenerios. We can assert that all containers have been used in the last x mins
etc. Not removing this for now. We can always revisit this. I think this is a
good stat to have in container info.
{quote}setState : Having setter method in ContainerInfo, this one is just a
concern which was raised in [1] and [2] comment.
{quote}
We will have to have a client facing class which is different from the one we
use in the server. We will have to fix that in a different JIRA. I am going to
file or update one of the related JIRAs.
{quote}This class is not referred/used anywhere, is this required? Am I missing
something?
{quote}
Started using this class.
{quote}Line:131 We are not using final long cacheSize anywhere, can be removed
from the constructor
{quote}
Fixed.
{quote}Line:433 The TODO can be removed, it has been fixed in HDFS-12751
{quote}
Fixed.
{{TestContainerStateManager.java}} – Not able to correlate the comments to the
line number in comments.
{quote}Line 52: unused imports.
{quote}
Fixed.
{quote}Line 118: can we use FAILED_TO_CHANGE_CONTAINER_STATE without adding
FAILED_TO_UPDATE_CONTAINER_STATE?
{quote}
Fixed.
{quote}Line 119: CONTAINER_MAPS_NO_SUCH_STATE is not used anywhere, can we
remove it?
{quote}
Fixed.
{quote}Line 120: can we use CONTAINER_EXISTS without adding
CONTAINER_ID_NOT_UNIQUE
{quote}
Fixed.
{quote}Line 214-218: I think the revert logic inside ContainerAttribute#update
should have covered the case here. The lifeCycleStateMap revert here can be
removed.
{quote}
We don't know for sure what value is being updated in the containerInfo. So we
need this here.
{quote}Line 334: can we use currentSet.retainAll(sets[x]); without adding a
helper intersectSets()
{quote}
I looked at using that function, however the semantics provided by that
function is very destructive for us.
We have a couple of sets, we just need to pick some values from those, without
modifying those original sets. The ratainAll modifies the sets, which will
create too much object churn.
{quote}Line 119-121: lastUsedMap can be a map of String to ContainerID.
lastusedMap and containerCount can be wrapped into ContainerStateMap class with
the same lock for protection and better isolation.
{quote}
Fixed.
{quote}Line 183-189: we should add this comments to the Ozone.proto we well
which addes the containerID field as a int32 (not int64) for SCMContainerInfo.
{quote}
Fixed.
{quote}Line 310: Do we want to add a WARN log when the containerCount wrap as a
32-bit integer.
{quote}
Fixed by promoting to a long.
{quote}The logic to determine if the container have enough space is different
from previous one. Can you elaborate on it?
{quote}
Fixed, after allocating a block, we now update the allocatedBytes.
{quote}Unused class can be removed.
{quote}
Fixed, not unused any more.
> Ozone: Remove the Priority Queues used in the Container State Manager
> ---------------------------------------------------------------------
>
> Key: HDFS-12522
> URL: https://issues.apache.org/jira/browse/HDFS-12522
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Reporter: Anu Engineer
> Assignee: Anu Engineer
> Priority: Major
> Attachments: HDFS-12522-HDFS-7240.001.patch,
> HDFS-12522-HDFS-7240.002.patch
>
>
> During code review of HDFS-12387, it was suggested that we remove the
> priority queues that was used in ContainerStateManager. This JIRA tracks that
> issue.
--
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
