Skip to content

merging latest avatica - #157

Open
gkanade wants to merge 33 commits into
apache:masterfrom
gkanade:master
Open

merging latest avatica#157
gkanade wants to merge 33 commits into
apache:masterfrom
gkanade:master

Conversation

@gkanade

@gkanadegkanade commented Mar 29, 2016

Copy link
Copy Markdown

No description provided.

@JamesRTaylor

JamesRTaylor commented Apr 5, 2016

Copy link
Copy Markdown
Contributor

@codymarcel - would it be possible for you to review this pull as Mujtaba is on PTO?

@codymarcel

codymarcel commented Apr 5, 2016

Copy link
Copy Markdown
Contributor

Yea, I'll take a look. What is the associated Jira?

On Mon, Apr 4, 2016 at 8:02 PM, James Taylor notifications@github.com
wrote:

@codymarcelhttps://github.com/codymarcel - would it be possible for
you to review this pull as Mujtaba is on PTO?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#157 (comment)

codymarcel
codymarcel reviewed Apr 5, 2016
View reviewed changes
Comment threadphoenix-pherf/src/main/java/org/apache/phoenix/pherf/Pherf.java
// Schema and Data Load
if (preLoadData) {
logger.info("\nStarting Data Load...");
System.out.print("Starting Data Load ...");

@codymarcelcodymarcelApr 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should be able to use the logger config for stdout instead.

@JamesRTaylor

JamesRTaylor commented Apr 5, 2016

Copy link
Copy Markdown
Contributor

@gkanade - please file a JIRA and update you git commit message to include the JIRA (i.e. PHOENIX-#### Update Pherf to support ....).

codymarcel
codymarcel reviewed Apr 5, 2016
View reviewed changes
Comment threadphoenix-pherf/src/main/java/org/apache/phoenix/pherf/workload/WriteWorkload.java

last = start = System.currentTimeMillis();
String sql = buildSql(columns, tableName);
//System.out.print("going to create statement");

@codymarcelcodymarcelApr 5, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

remove commented out code.

@codymarcel

codymarcel commented Apr 5, 2016

Copy link
Copy Markdown
Contributor

General Comments.

  1. Please add some unit tests. Especially around the instanceResolver and RulesApplier. We user https://github.com/apache/phoenix/blob/master/phoenix-pherf/src/test/resources/scenario/test_scenario.xml as both test base and doc for supported features.
  2. Formatting needs some work across the board. Make sure you have the formatter for the project applied and your ide is set to convert tabs to spaces.
  3. Remove all code that's in comments.

@codymarcel

codymarcel commented Apr 13, 2016

Copy link
Copy Markdown
Contributor

@gkanade Any updates on this?

This was referenced Aug 16, 2021
Closed
Merged
This was referenced Sep 9, 2021
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gkanade@JamesRTaylor@codymarcel