[
https://issues.apache.org/jira/browse/KNOX-1064?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16190014#comment-16190014
]
Phil Zampino commented on KNOX-1064:
------------------------------------
1. It could be a ConcurrentHashMap, but do you see a reason why it ought to be?
Instances of this class are not handled by multiple threads concurrently.
2. Since AmbariComponent is only used by the Ambari service discovery
implementation, there is no need to expose them outside of the package.
3. AmbariServiceDiscovery
a. 3.1 serviceConfiguration Map was leftover from a prior change (thanks)
b. 3.2 We may indeed be facing the fact that the Ambari v1 API will be
discontinued, but I’m looking at that as an implementation detail.
The truth is that the resource URIs are pretty tightly coupled with the content
they provide.
Externalizing the URIs will have little impact on our ability to accommodate
future versions of the API.
I’ll deal with this separately in the context of handling multiple Ambari API
versions.
c. 3.3 aliasService is used in the invokeREST method
d. 3.4 I’ve left it there to record the alternative host property name, but I
suppose it can be removed
4. SimpleDescriptorHandler
a. 4.1 Are * imports frowned-upon? Is there some threshold whereby they’re
acceptable, or they’re never acceptable?
b. 4.2 Yes, it could be wrapped in a BufferedWriter
c. 4.3 Good catch; I will move the close to a finally block
5. There is no notion of closing a File, so I’ll assume you mean closing any
associated streams which are opened.
I will add this in ServiceURLPropertyConfig, where the streams are read.
--
Phil
On 10/3/17, 12:05 PM, "Sandeep More (JIRA)" <j...@apache.org> wrote:
[
https://issues.apache.org/jira/browse/KNOX-1064?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16189905#comment-16189905
]
Sandeep More commented on KNOX-1064:
------------------------------------
Great, thanks for the patch Phil, sorry for the delay in reviewing.
Since the patch is a bit longer (for me) I reviewed the entire file in some
cases than the diff as it was easier to get the context with full file. Also,
the suggestion I have are not major they are minor nit-picks patch overall
looks great (I haven't yet tested it)
Comments:
1. in AmbariCluster class, serviceConfigurations can be a ConcurrentHashMap
insted of HashMap
2. AmbariComponent class, any reason why the methods were made package
protected ?
3. AmbariServiceDiscovery class,
3.1 serviceConfiguration Map is not used
3.2 Given that AMBARI_CLUSTERS_URI and AMBARI_HOSTROLES_URI are Ambari
specific paths which could change in th future, do you think we can move it to
ext. mapping file
3.3 aliasService variable is not used
3.4 You can remove the comment on line # 147
4. SimpleDescriptorHandler class
4.1 line # 25 and 28 have * imports.
4.2 line # 124, may be use BufferedWriter to wrap FileWriter to be more
effecient
4.3 The Writer needs to be closed in finally block to prevent leaking
in case of exception.
5 AmbariDynamicServiceURLCreator class
5.1 mappingConfigFile file object should be closed.
5.2 at line # 64, we could close FileInputStream after it's dome reading
5.3 at line 69, same as 5.2
> Externalize Hadoop Service Configuration Details and Service URL Creation
> -------------------------------------------------------------------------
>
> Key: KNOX-1064
> URL: https://issues.apache.org/jira/browse/KNOX-1064
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
> Affects Versions: 0.14.0
> Reporter: Phil Zampino
> Assignee: Phil Zampino
> Labels: kip-8
> Attachments: KNOX-1064-001.patch, KNOX-1064-002.patch,
KNOX-1064-003.patch, KNOX-1064.patch
>
>
> For the sake of flexibility in terms of the services supported by service
discovery and topology generation, the following should be externalized from
the code:
> # The set and mapping of service components and their respective
configuration details.
> # The service URL generation details.
> The goal is to be able to add/modify these details without having to
modify source code.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
> Externalize Hadoop Service Configuration Details and Service URL Creation
> -------------------------------------------------------------------------
>
> Key: KNOX-1064
> URL: https://issues.apache.org/jira/browse/KNOX-1064
> Project: Apache Knox
> Issue Type: Bug
> Components: Server
> Affects Versions: 0.14.0
> Reporter: Phil Zampino
> Assignee: Phil Zampino
> Labels: kip-8
> Attachments: KNOX-1064-001.patch, KNOX-1064-002.patch,
> KNOX-1064-003.patch, KNOX-1064.patch
>
>
> For the sake of flexibility in terms of the services supported by service
> discovery and topology generation, the following should be externalized from
> the code:
> # The set and mapping of service components and their respective
> configuration details.
> # The service URL generation details.
> The goal is to be able to add/modify these details without having to modify
> source code.
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
