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.

Sign up for GitHub

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

Jump to bottom

Mcc cdh5.10.1 #1

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Mcc cdh5.10.1 #1

wants to merge 17 commits into from

Conversation

balachandrapai
Copy link

Jan, Please review the changes

@HorizonNet
Copy link
Member

Please check the build results. One of the tests fails. Also, your pull requests includes the .idea directory. This one should be removed.

@HorizonNet HorizonNet self-assigned this Jun 26, 2017
HorizonNet
HorizonNet requested changes Jun 26, 2017
View reviewed changes
Copy link
Member

@HorizonNet HorizonNet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments. Please try to use a proper formatting.

pom.xml Outdated
@@ -22,77 +22,77 @@
<dependency>
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-common</artifactId>
<version>2.5.0-cdh5.3.0</version>
<version>2.6.0-cdh5.10.1</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put this into a variable. This makes later changes easier.

pom.xml Outdated
</dependency>
<dependency>
<groupId>org.apache.hadoop</groupId>
<artifactId>hadoop-common</artifactId>
<version>2.5.0-cdh5.3.0</version>
<version>2.6.0-cdh5.10.1</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dito.

pom.xml Outdated
<type>test-jar</type>
<classifier>tests</classifier>
</dependency>
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-client</artifactId>
<version>0.98.6-cdh5.3.0</version>
<version>1.2.0-cdh5.10.1</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put this into a variable. This makes later changes easier.

pom.xml Outdated
</dependency>
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-server</artifactId>
<version>0.98.6-cdh5.3.0</version>
<version>1.2.0-cdh5.10.1</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dito.

pom.xml Outdated
<type>test-jar</type>
</dependency>
<dependency>
<groupId>org.apache.hbase</groupId>
<artifactId>hbase-server</artifactId>
<version>0.98.6-cdh5.3.0</version>
<version>1.2.0-cdh5.10.1</version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dito.

src/resources/krb5.conf Outdated
@@ -0,0 +1,29 @@
# Configuration snippets may be placed in this directory as well
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need a Kerberos configuration file?

src/test/java/org/apache/hadoop/hbase/client/HBaseMCCTest.java Outdated
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the comment.

src/test/java/org/apache/hadoop/hbase/client/HBaseMCCTest.java Outdated
import java.util.HashMap;
import java.util.Map;

import static org.junit.Assert.assertFalse;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please try not to use static imports.

src/test/java/org/apache/hadoop/hbase/client/HBaseMCCTest.java Outdated
@@ -0,0 +1,154 @@
package org.apache.hadoop.hbase.client;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reformat the whole file. There are a lot of empty lines in between.

src/test/java/org/apache/hadoop/hbase/client/HBaseMCCTest.java Outdated
}


// //failover Cluster2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these lines are commented out?

HorizonNet
HorizonNet requested changes Jun 30, 2017
View reviewed changes
Copy link
Member

@HorizonNet HorizonNet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some additional comments.

src/main/java/org/apache/hadoop/hbase/client/HTableMultiCluster.java Outdated
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.HTableDescriptor;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use * imports.

src/main/java/org/apache/hadoop/hbase/test/MultiHBaseClusterClientTest.java Outdated

import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.HBaseConfiguration;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.client.*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use * imports.

src/main/java/org/apache/hadoop/hbase/test/MultiHBaseClusterClientTest.java Outdated
@@ -1,41 +1,32 @@
package org.apache.hadoop.hbase.client;
package org.apache.hadoop.hbase.test;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change of the package name?

src/main/java/org/apache/hadoop/hbase/test/MultiHBaseClusterClientTest.java Outdated



Assert.assertEquals(Boolean.FALSE, result.isEmpty());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't you use a simple false?

src/main/java/org/apache/hadoop/hbase/test/MultiHBaseClusterClientTest.java Outdated
@@ -142,13 +101,10 @@ public void writeToMCC(){
Get get1 = new Get(Bytes.toBytes("row4"));
Result result = multiTable.get(get1);

assertFalse(result.isEmpty());
Assert.assertEquals(Boolean.FALSE, result.isEmpty());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto ... or just Assert.assertFalse.

pom.xml Outdated
@@ -15,84 +15,87 @@

<maven.compiler.target>1.7</maven.compiler.target>
<maven.compiler.source>1.7</maven.compiler.source>
<hadoop.version>2.6.0-cdh5.10.1</hadoop.version>
<hbase.version>1.2.0-cdh5.10.1</hbase.version>
<cloudera-manager.version>5.10.1</cloudera-manager.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename this one to cdh.version.

pom.xml Outdated
@@ -15,84 +15,87 @@

<maven.compiler.target>1.7</maven.compiler.target>
<maven.compiler.source>1.7</maven.compiler.source>
<hadoop.version>2.6.0-cdh5.10.1</hadoop.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid duplication use the variable for CDH in this one.

pom.xml Outdated
@@ -15,84 +15,87 @@

<maven.compiler.target>1.7</maven.compiler.target>
<maven.compiler.source>1.7</maven.compiler.source>
<hadoop.version>2.6.0-cdh5.10.1</hadoop.version>
<hbase.version>1.2.0-cdh5.10.1</hbase.version>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HorizonNet HorizonNet HorizonNet requested changes

@tantalus1984 tantalus1984 Awaiting requested review from tantalus1984

Assignees

@HorizonNet HorizonNet

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@balachandrapai @HorizonNet