Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CDAP-7679] add hdfs datanode count to operational stats #8241

Closed
wants to merge 3 commits into from

Conversation

@liu-joe
Copy link
Contributor

liu-joe commented Feb 28, 2017

No description provided.

@liu-joe liu-joe added the 4.2 label Feb 28, 2017
@@ -120,6 +124,14 @@
<artifactId>jcl-over-slf4j</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents</groupId>

This comment has been minimized.

Copy link
@chtyim

chtyim Mar 22, 2017

Contributor

Is there any specific functionalities you needed from the httpclient that is missing from the java HttpURLConnection? If not, please use the Java one instead. There is also a set of classes in CDAP for simplifying the interaction. Find usage of the HttpRequests class as a starting point.

This comment has been minimized.

Copy link
@chtyim

chtyim Mar 22, 2017

Contributor

The main reason of avoiding the httpclient library is due to compatibility issue.

if (HAUtil.isHAEnabled(conf, getNameService())) {
URL haWebURL = getHAWebURL();
if (haWebURL != null) {
return haWebURL.toString();

This comment has been minimized.

Copy link
@chtyim

chtyim Apr 4, 2017

Contributor

Why returns String. Should use URL to represent url.

This comment has been minimized.

Copy link
@liu-joe

liu-joe Apr 4, 2017

Author Contributor

Using a non primitive type in the JMX bean classes is a lot more complicated, so we just represent it as a string

}
lastCollectFailed = true;
}
return null;

This comment has been minimized.

Copy link
@chtyim

chtyim Apr 4, 2017

Contributor

You should annotate the method with @Nullable if this method can return null. Also add javadoc to tell what does it mean when null is returned.

lastCollectFailed = false;
} catch (Exception e) {
// TODO: remove once CDAP-7887 is fixed
if (!lastCollectFailed) {

This comment has been minimized.

if (1 == nameservices.size()) {
return Iterables.getOnlyElement(nameservices);
}
throw new IllegalStateException("Found multiple nameservices configured in HDFS. CDAP currently does not support " +

This comment has been minimized.

Copy link
@chtyim

chtyim Apr 4, 2017

Contributor

This error message is misleading. It is only the operation stats collection doesn't support HDFS federation instead of CDAP itself.

@chtyim

This comment has been minimized.

Copy link
Contributor

chtyim commented Apr 4, 2017

LGTM

@bdmogal

This comment has been minimized.

Copy link
Contributor

bdmogal commented Apr 4, 2017

@liu-joe , let's merge this to release/4.1

@liu-joe liu-joe changed the base branch from develop to release/4.1 Apr 4, 2017
@liu-joe liu-joe changed the base branch from release/4.1 to develop Apr 4, 2017
@liu-joe liu-joe closed this Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.