[
https://issues.apache.org/jira/browse/HDFS-14547?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16874493#comment-16874493
]
Erik Krogen commented on HDFS-14547:
------------------------------------
[~LiJinglun] I just took a look, thanks a lot for this work. It is very
sensible. I have a few comments throughout but they are all fairly minor
implementation or style details.
* {{nsSsCounts}} and {{tsCounts}} can be package-private instead of protected
* I don't think we need to define {{CounterFunc}}, we can simply use
{{Consumer<EnumCounters<T>>}}
* Some Javadoc on {{QuotaCounts#modify()}} would be nice to explain why it is
necessary
* In {{QuotaCounts#setTypeSpaces()}}, instead of checking {{allEqual}}, can we
just check reference equality with {{STORAGE_TYPE_DEFAULT}} and
{{STORAGE_TYPE_RESET}}? This is like {{Builder#typeSpaces()}}.
* I think {{anyNsSsCountGreaterOrEqual}} is currently wrong, instead of
{code}
if (nsSsCounts == QUOTA_DEFAULT && val >= 0) {
return false;
}
{code}
it should be
{code}
if (nsSsCounts == QUOTA_DEFAULT) {
return val <= 0;
} else if (nsSsCounts == QUOTA_RESET) {
return val <= HdfsConstants.QUOTA_RESET;
}
{code}
(and similar for {{anyTypeSpaceCountGreaterOrEqual}})
* There's a lot of code duplicated between {{Builder#nameSpace()}},
{{Builder#storageSpace()}}, {{setNameSpace()}}, and {{setStorageSpace()}}. If
we write a new helper method like below, we can leverage it for all 4:
{code}
private static EnumCounters<Quota> setQuotaCounter(
EnumCounters<Quota> inputCounts, Quota quotaToSet, Quota otherQuota,
long val) {
if (val == HdfsConstants.QUOTA_RESET
&& inputCounts.get(otherQuota) == HdfsConstants.QUOTA_RESET) {
return QUOTA_RESET;
} else if (val == 0 && inputCounts.get(otherQuota) == 0) {
return QUOTA_DEFAULT;
} else {
return modify(inputCounts, ec->ec.set(quotaToSet, val));
}
}
{code}
* In {{deepCopyEnumCounter}}, we should make sure that we give a type parameter
for {{EnumCounters}}
* The constant definitions can be package-private or private, I don't think
they need to be public? Also add type parameters to the constructor. Also,
there's no need for a separate {{static}} block, why not just initialize them
alongside their definition?
* Instead of saying {{See @link EnumCounters}}, you can just do {{@see
EnumCounters}}
* Constructor for {{ConstEnumException}} should be private
* {{CONST_ENUM_EXCEPTION}} should probably be private, and maybe have a more
helpful message
* Can you add {{@Override}} tags to all of the methods in
{{ConstEnumCounters}}, and probably remove the Javadoc since they aren't really
accurate? (all of them just throw the modification exception)
* Thanks for adding the new {{TestQuotaCounts}} test. It's a little concerning
that there were no tests before. Can you please use {{assertEquals}} and
{{assertNonEquals}} as appropriate instead of {{assertTrue(... == ...)}} ?
Though the one test is nice, this is a pretty big logic change, I wonder if you
could add a few more test cases? It would also be helpful if they were broken
into separate test cases instead of combined into one method, to make it easier
to see what is being tested.
> DirectoryWithQuotaFeature.quota costs additional memory even the storage type
> quota is not set.
> -----------------------------------------------------------------------------------------------
>
> Key: HDFS-14547
> URL: https://issues.apache.org/jira/browse/HDFS-14547
> Project: Hadoop HDFS
> Issue Type: Improvement
> Affects Versions: 3.1.0
> Reporter: Jinglun
> Assignee: Jinglun
> Priority: Major
> Attachments: HDFS-14547-design, HDFS-14547-patch003-Test Report.pdf,
> HDFS-14547.001.patch, HDFS-14547.002.patch, HDFS-14547.003.patch,
> HDFS-14547.004.patch, HDFS-14547.005.patch
>
>
> Our XiaoMi HDFS is considering upgrading from 2.6 to 3.1. We notice the
> storage type quota 'tsCounts' is instantiated to
> EnumCounters<StorageType>(StorageType.class), so it will cost a long[5] even
> if we don't have any storage type quota on this inode(only space quota or
> name quota).
> In our cluster we have many dirs with quota and the NameNode's memory is in
> tension, so the additional cost will be a problem.
> See DirectoryWithQuotaFeature.Builder().
>
> {code:java}
> class DirectoryWithQuotaFeature$Builder {
> public Builder() {
> this.quota = new QuotaCounts.Builder().nameSpace(DEFAULT_NAMESPACE_QUOTA).
> storageSpace(DEFAULT_STORAGE_SPACE_QUOTA).
> typeSpaces(DEFAULT_STORAGE_SPACE_QUOTA).build();// set default value -1.
> this.usage = new QuotaCounts.Builder().nameSpace(1).build();
> }
> public Builder typeSpaces(long val) {// set default value.
> this.tsCounts.reset(val);
> return this;
> }
> }
> class QuotaCounts$Builder {
> public Builder() {
> this.nsSsCounts = new EnumCounters<Quota>(Quota.class);
> this.tsCounts = new EnumCounters<StorageType>(StorageType.class);
> }
> }
> class EnumCounters {
> public EnumCounters(final Class<E> enumClass) {
> final E[] enumConstants = enumClass.getEnumConstants();
> Preconditions.checkNotNull(enumConstants);
> this.enumClass = enumClass;
> this.counters = new long[enumConstants.length];// new a long array here.
> }
> }
> {code}
> Related to HDFS-14542.
>
--
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
