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

remove enums #761

Merged
merged 3 commits into from Dec 6, 2016
Merged

remove enums #761

merged 3 commits into from Dec 6, 2016

Conversation

shintasmith
Copy link
Contributor

Remove enums implementation and support

@shintasmith
Copy link
Contributor Author

@usnavi @ChandraAddala @izrik @drenalin23 pls review

@shintasmith
Copy link
Contributor Author

Oh, btw, I need someone who knows about how Enums are implemented in blueflood-elasticsearch to scrutinize the code changes there. I have to admit, I'm shooting in the dark! 😬

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.02%) to 75.01% when pulling 4c054f6 on shintasmith:rm_enums into 1faece3 on rackerlabs:master.

Copy link
Contributor

@usnavi usnavi left a comment

Choose a reason for hiding this comment

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

done.

@@ -69,7 +69,7 @@ public void writeAllAstyanaxReadAllDatastax() throws IOException {

Table<Locator, String, String> meta = HashBasedTable.create();
meta.put( l0, CACHE_KEY, RollupType.GAUGE.toString() );
meta.put( l1, CACHE_KEY, RollupType.ENUM.toString() );
meta.put( l1, CACHE_KEY, RollupType.SET.toString() );
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we adding this? I assume its a statsd type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not adding it. I'm just replacing ENUMs with SETs.

@ChandraAddala
Copy link
Contributor

The only comment I have is, we havent removed column family definitions of column families corresponding to enums.

@@ -142,83 +133,16 @@
//token paths matching query, which also have a next level.
tokenInfoBuilder.addTokenPathWithNextLevel(metricIndexData.getTokenPathsWithNextLevel());

Set<String> completeMetricsMatchingQueryPrevLevel = metricIndexData.getCompleteMetricNamesAtBaseLevel();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary for sub-metrics? any thoughts @caddala?

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to know if it is a complete metric name at base level to look for enums. If there are no enums we dont need it.

But I think we can adjust the regex expression a little bit, now that we dont need that information. We can create another story to look into this API after we cleanup enums.

@shintasmith shintasmith merged commit 0e56949 into rax-maas:master Dec 6, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.0%) to 75.077% when pulling a6e5ce5 on shintasmith:rm_enums into 98e273b on rackerlabs:master.

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.

None yet

4 participants