Skip to content

Simplify information in IncrementalIndex#2169

Merged
jon-wei merged 1 commit intoapache:masterfrom
navis:refactor-incremental-index3
Jan 12, 2016
Merged

Simplify information in IncrementalIndex#2169
jon-wei merged 1 commit intoapache:masterfrom
navis:refactor-incremental-index3

Conversation

@navis
Copy link
Copy Markdown
Contributor

@navis navis commented Dec 29, 2015

For

private final Map<String, Integer> metricIndexes;
private final Map<String, String> metricTypes;
private final ImmutableList<String> metricNames;

and

private final LinkedHashMap<String, Integer> dimensionOrder;
private final DimensionHolder dimValues;
private final CopyOnWriteArrayList<String> dimensions;

each group can be merged into single map

@navis navis force-pushed the refactor-incremental-index3 branch from 1a867ed to 7b667c8 Compare December 29, 2015 04:47
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.

why change this from metrics?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I used metrics for metrics. Ok, I'll revert that.

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Dec 29, 2015

I'm in favor of this PR but I have some outstanding comments

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.

The unofficial convention is to put static classes at the end of class definitions, if I recall from @himanshug 's formatting requests on other PRs

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ok

@drcrallen
Copy link
Copy Markdown
Contributor

Very reasonable and easier to read changes. Can you use the getters for the DimensionDesc class fields instead of the fields directly?

@navis navis force-pushed the refactor-incremental-index3 branch from ba71ee6 to 5d0705b Compare January 4, 2016 23:45
@nishantmonu51
Copy link
Copy Markdown
Member

👍

@jon-wei
Copy link
Copy Markdown
Contributor

jon-wei commented Jan 9, 2016

👍 Cool PR, looks good to me

@jon-wei
Copy link
Copy Markdown
Contributor

jon-wei commented Jan 9, 2016

@navis Can you squash your commits?

@jon-wei
Copy link
Copy Markdown
Contributor

jon-wei commented Jan 12, 2016

@navis I'm working on some non-String typing changes to IncrementalIndex, and I want to get this PR in before I submit a PR for that, can you squash the commits if you don't have any more changes?

@fjy @drcrallen @nishantmonu51 I plan on merging this after the squash, do you have any more feedback on this?

@navis navis force-pushed the refactor-incremental-index3 branch from 5d0705b to 2f9c45f Compare January 12, 2016 01:11
@navis
Copy link
Copy Markdown
Contributor Author

navis commented Jan 12, 2016

@jon-wei Squashed

@drcrallen
Copy link
Copy Markdown
Contributor

is navis@3f0c8cd showing up for anyone else in this PR?

@drcrallen
Copy link
Copy Markdown
Contributor

yeah, I think @navis you may have grabbed more commits than you meant to.

@jon-wei
Copy link
Copy Markdown
Contributor

jon-wei commented Jan 12, 2016

I see that CliPeon commit as well

@navis navis force-pushed the refactor-incremental-index3 branch from 2f9c45f to 976ebc4 Compare January 12, 2016 01:18
@navis
Copy link
Copy Markdown
Contributor Author

navis commented Jan 12, 2016

Sorry, my bad.

1 similar comment
@navis
Copy link
Copy Markdown
Contributor Author

navis commented Jan 12, 2016

Sorry, my bad.

jon-wei added a commit that referenced this pull request Jan 12, 2016
Simplify information in IncrementalIndex
@jon-wei jon-wei merged commit 419fa5a into apache:master Jan 12, 2016
@fjy fjy modified the milestone: 0.9.0 Feb 4, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants