[
https://issues.apache.org/jira/browse/YARN-3663?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15968218#comment-15968218
]
Carlo Curino commented on YARN-3663:
------------------------------------
Thanks [~giovanni.fumarola] for the updated patch.
Conf:
# You set
{{YarnConfiguration.DEFAULT_FEDERATION_STATESTORE_SQL_MAXCONNECTIONS}} to 1.
Isn't this too small? What values you commonly use? I would put a conservative
but not overly tight value, otherwise users are forced to learn/modify more
params.
# We should omit {{yarn.federation.state-store.sql.max-connections} from
yarn-default.xml like you do for all other params in this patch.
In {{SQLFederationStateStore}}
# For most fields except {{CALL_SP_GET_ALL_POLICY_CONFIGURATIONS}} you simply
use the plural to differentiate the getter of one or all items. Be consistent
(e.g., remove the ALL here, or add it everywhere else)
# Minor: userNameDB --> userName, passwordDB --> password
# When you throw the exceptions (e.g., subcluster registration), it might be
nice in the message to include the sub-clusterID / ip or any other info one can
use to debug.
# Can you comment on why we are using: {{FederationStateStoreErrorCode}}?
They don't seem to be connected to SQL error codes, and they are not used
anywhere else (we normally use named exception, which are easier to
understand/track).
# at line 277-278: formatting
# We should try to remove redundance, e.g., you have lots of things that look
like this:
{code}
try {
FederationMembershipStateStoreInputValidator
.validateGetSubClusterInfoRequest(subClusterRequest);
} catch (FederationStateStoreInvalidInputException e) {
FederationStateStoreUtils.logAndThrowInvalidInputException(LOG,
"Unable to obtain the infomation about the SubCluster. "
+ e.getMessage());
}
{code}
They could be factored out to
{{FederationMembershipStateStoreInputValidator.validate(subClusterRequest)}}
where the type of input param is used to differentiate the method, and the
logAndThrowInvalidInputException is done on that side. Same goes for
{{checkSubClusterInfo}}.
# Similarly to the above we should try to factor out the very repetitive code
to create connection/statements, set params, run, and throw. I don't have a
specific advise on this, but the code is mostly copy and paste, which we should
avoid.
# Move the {{fromStringToSubClusterState}} to the SubclusterState class (and
call it fromString().
# Why {{getSubCluster}} and {{getSubClusters}} use different mechanics for
return values? (registered params vs ResultSet)? Might be worth to be
consistent (probably using ResultSet).
# Line 540: Is this behavior (overwrite existing) consistent with general
YARN? (I think so, but want to check)
# Some of the log are a bit vague {{LOG.debug("Got the information about the
specified application }} say spefically what info where gotten
# if you use {{LOG.debug}} consider to prefix it with a check if we are in
debug mode (save time/objects creations for the String that are then not used).
# You have several {{ if (cstmt.getInt(2) != 1) }} ROWCOUNT checks. This
mix the no tuple where changed to multiple tuple where changed. Distinguishing
the two cases, might help debug (do we have duplicates in DB, or the entry was
not found). (Not mandatory, just somethign to consider)
# {{setPolicyConfiguration}} You are doing {{cstmt.setBytes(3, new
byte[policyConf.getParams().remaining()]);}} which adds an empty byte[] instead
of what is coming in input.
# {{getCurrentVersion}} and {{loadVersion}} throw a NotSupportedException or
something of the sort, a silent return null is easy to confuse people. (I know
the full version will be in V2, let's just have a clear breakage if someone try
to use this methods).
(.... to be continued ...)
> Federation State and Policy Store (DBMS implementation)
> -------------------------------------------------------
>
> Key: YARN-3663
> URL: https://issues.apache.org/jira/browse/YARN-3663
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: nodemanager, resourcemanager
> Affects Versions: YARN-2915
> Reporter: Giovanni Matteo Fumarola
> Assignee: Giovanni Matteo Fumarola
> Attachments: YARN-3663-YARN-2915.v1.patch,
> YARN-3663-YARN-2915.v2.patch
>
>
> This JIRA tracks a SQL-based implementation of the Federation State and
> Policy Store, which implements YARN-3662 APIs.
--
This message was sent by Atlassian JIRA
(v6.3.15#6346)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
