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

[core] Use longs instead of ints to support larger key spaces. #911

Closed
wants to merge 4 commits into from
Closed

[core] Use longs instead of ints to support larger key spaces. #911

wants to merge 4 commits into from

Conversation

mairbek
Copy link
Contributor

@mairbek mairbek commented Jan 24, 2017

No description provided.

risdenk
risdenk reviewed Jan 25, 2017
View reviewed changes
Copy link
Collaborator

@risdenk risdenk left a comment

Choose a reason for hiding this comment

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

@mairbek Looks good just one line comment. The headers should be updated with the new year if necessary.

core/src/main/java/com/yahoo/ycsb/generator/UniformGenerator.java Outdated
@@ -48,7 +48,8 @@ public UniformGenerator(Collection<String> values)
@Override
public String nextValue()
{
_laststring = _values.get(_gen.nextValue());
int val = _gen.nextValue().intValue();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this line necessary?

@risdenk risdenk self-assigned this Jan 25, 2017
@risdenk risdenk added this to the 0.13.0 milestone Jan 25, 2017
@risdenk
Copy link
Collaborator

risdenk commented Jan 25, 2017

@mairbek thanks for the PR. Just for reference this looks related to #664 and may supercede PR #126

risdenk
risdenk requested changes Jan 25, 2017
View reviewed changes
core/src/main/java/com/yahoo/ycsb/generator/UniformGenerator.java Outdated
@@ -48,7 +48,7 @@ public UniformGenerator(Collection<String> values)
@Override
public String nextValue()
{
_laststring = _values.get(_gen.nextValue());
_laststring = _values.get(_gen.nextValue().intValue());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason we are doing .intValue()? Sorry I wasn't clear earlier with my comment. Should we be using longs everywhere?

Copy link
Contributor Author

@mairbek mairbek Jan 25, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The size of ArrayList is int, so limiting the value.

Also UniformGenerator doesn't seem to be used so maybe we can consider removing it.

core/src/main/java/com/yahoo/ycsb/generator/AcknowledgedCounterGenerator.java Outdated
@@ -1,5 +1,5 @@
/**
* Copyright (c) 2015 YCSB contributors. All rights reserved.
* Copyright (c) 2017 YCSB contributors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

These should be 2015-2017 and not a replacement. I should have been more clear.

@mairbek
Copy link
Contributor Author

mairbek commented Jan 25, 2017

Thanks for the review!

Thanks for pointing me to #126, it seems to be very similar.

I will update measurements related code too. Looking at OneMeasurementHistogram.java:60, number of operations is an Integer so that code could potentially go wrong. At this point we are not running more then 1B operations so I never saw it breaking before.

@mairbek mairbek force-pushed the longkeys branch 2 times, most recently from 668da1a to 4ed3487 Compare January 26, 2017 01:12
risdenk
risdenk requested changes Jan 26, 2017
View reviewed changes
Copy link
Collaborator

@risdenk risdenk left a comment

Choose a reason for hiding this comment

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

Two minor header fixes needed. The changes look good I'll pull it down later today or tomorrow and check it further.

core/src/test/java/com/yahoo/ycsb/generator/AcknowledgedCounterGeneratorTest.java Outdated
@@ -1,5 +1,5 @@
/**
* Copyright (c) 2015 YCSB contributors. All rights reserved.
* Copyright (c) 2016-2017 YCSB contributors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be 2015-2017

core/src/main/java/com/yahoo/ycsb/workloads/RestWorkload.java Outdated
@@ -1,5 +1,5 @@
/**
* Copyright (c) 2016 YCSB contributors. All rights reserved.
* Copyright (c) 2017 YCSB contributors. All rights reserved.
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be 2016-2017

@mairbek
Copy link
Contributor Author

mairbek commented Jan 26, 2017

Updated the headers. Squashed all header related commits as well...

@risdenk risdenk changed the title Use longs instead of ints to support larger key spaces. [core] Use longs instead of ints to support larger key spaces. Jan 27, 2017
@risdenk
Copy link
Collaborator

risdenk commented Jan 27, 2017

This change looks good to me. @busbey or @cmccoy do you guys have any comments? I know you reviewed #126 and #432. This is just for the larger key spaces and doesn't touch operations.

@risdenk risdenk requested review from busbey and cmccoy January 27, 2017 16:44
@risdenk
Copy link
Collaborator

risdenk commented Feb 3, 2017

@mairbek - Sorry this looks like it conflicted with the checkstyle changes for core #895. Can you update your PR?

Merge branch 'master' into longkeys
e15d9f2 
# Conflicts:
#	core/src/main/java/com/yahoo/ycsb/generator/AcknowledgedCounterGenerator.java
#	core/src/main/java/com/yahoo/ycsb/generator/CounterGenerator.java
#	core/src/main/java/com/yahoo/ycsb/generator/HotspotIntegerGenerator.java
#	core/src/main/java/com/yahoo/ycsb/generator/SequentialGenerator.java
#	core/src/main/java/com/yahoo/ycsb/generator/UniformGenerator.java
#	core/src/main/java/com/yahoo/ycsb/generator/UniformLongGenerator.java
#	core/src/main/java/com/yahoo/ycsb/measurements/OneMeasurementHistogram.java
#	core/src/main/java/com/yahoo/ycsb/measurements/OneMeasurementTimeSeries.java
#	core/src/main/java/com/yahoo/ycsb/measurements/exporter/JSONArrayMeasurementsExporter.java
#	core/src/main/java/com/yahoo/ycsb/measurements/exporter/JSONMeasurementsExporter.java
#	core/src/main/java/com/yahoo/ycsb/measurements/exporter/MeasurementsExporter.java
#	core/src/main/java/com/yahoo/ycsb/measurements/exporter/TextMeasurementsExporter.java
#	core/src/main/java/com/yahoo/ycsb/workloads/CoreWorkload.java
#	core/src/main/java/com/yahoo/ycsb/workloads/RestWorkload.java
@mairbek
Copy link
Contributor Author

mairbek commented Feb 8, 2017

Merged formatting changes.

@risdenk
Copy link
Collaborator

risdenk commented Mar 22, 2017

@mairbek sorry for the delay in this. I'm taking a look.

@mairbek
Copy link
Contributor Author

mairbek commented Apr 21, 2017

@risdenk ping, it would be nice to get this in to be able to run larger scale benchmarks

@manolama
Copy link
Collaborator

manolama commented Aug 5, 2017

Looked good, cleaned up the merge issues and committed in 59bc986.

@manolama manolama closed this Aug 5, 2017
@manolama manolama mentioned this pull request Sep 26, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@risdenk risdenk risdenk approved these changes

@busbey busbey Awaiting requested review from busbey

@cmccoy cmccoy Awaiting requested review from cmccoy

Assignees

@risdenk risdenk

Labels
improvement
Projects
None yet
Milestone
0.13.0
Development

Successfully merging this pull request may close these issues.
3 participants
@mairbek @risdenk @manolama