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

Storm-2203 Add a getAll method to KeyValueState interface #1798

Closed
wants to merge 10 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@aandis
Contributor

aandis commented Nov 25, 2016

Adds a getAll to InMemoryKeyValueState and RedisKeyValueState. I'll add tests once the changes are approved.

@HeartSaVioR

This comment has been minimized.

Show comment
Hide comment
@HeartSaVioR

HeartSaVioR Nov 30, 2016

Contributor

@aandis
Looks great. Could you add tests if possible? You may want to rename the prefix of the title to STORM-2203 so that it can be linked to JIRA issue automatically and review comments are written to work log.
Thanks in advance!

Contributor

HeartSaVioR commented Nov 30, 2016

@aandis
Looks great. Could you add tests if possible? You may want to rename the prefix of the title to STORM-2203 so that it can be linked to JIRA issue automatically and review comments are written to work log.
Thanks in advance!

@aandis aandis changed the title from Storm 2203 Add a getAll method to KeyValueState interface to Storm-2203 Add a getAll method to KeyValueState interface Nov 30, 2016

@aandis

This comment has been minimized.

Show comment
Hide comment
@aandis

aandis Dec 11, 2016

Contributor

Ping @HeartSaVioR @arunmahadevan can you review this?

Contributor

aandis commented Dec 11, 2016

Ping @HeartSaVioR @arunmahadevan can you review this?

@arunmahadevan

Looked at a high level and left a couple of comments. Once addressed will do one more pass.

@HeartSaVioR

This comment has been minimized.

Show comment
Hide comment
@HeartSaVioR

HeartSaVioR Dec 20, 2016

Contributor

@aandis Could you address review comments from @arunmahadevan?

Contributor

HeartSaVioR commented Dec 20, 2016

@aandis Could you address review comments from @arunmahadevan?

@aandis

This comment has been minimized.

Show comment
Hide comment
@aandis

aandis Dec 20, 2016

Contributor

@HeartSaVioR yep. will do in next couple of days.

Contributor

aandis commented Dec 20, 2016

@HeartSaVioR yep. will do in next couple of days.

@aandis

This comment has been minimized.

Show comment
Hide comment
@aandis

aandis Dec 26, 2016

Contributor

@HeartSaVioR @arunmahadevan ready for review.

Contributor

aandis commented Dec 26, 2016

@HeartSaVioR @arunmahadevan ready for review.

@arunmahadevan

Looks great overall. Have a few minor comments.

}
@Override
public Map.Entry<K, V> next() {

This comment has been minimized.

@arunmahadevan

arunmahadevan Dec 26, 2016

Contributor

next should throw a NoSuchElementException if there are no more elements. https://docs.oracle.com/javase/7/docs/api/java/util/Iterator.html#next()

in next add a check in the beginning.

if (!hasNext()) {
 throw new NoSuchElementException();
}
@arunmahadevan

arunmahadevan Dec 26, 2016

Contributor

next should throw a NoSuchElementException if there are no more elements. https://docs.oracle.com/javase/7/docs/api/java/util/Iterator.html#next()

in next add a check in the beginning.

if (!hasNext()) {
 throw new NoSuchElementException();
}
Show outdated Hide outdated ...c/main/java/org/apache/storm/redis/state/RedisKeyValueStateIterator.java
@arunmahadevan

This comment has been minimized.

Show comment
Hide comment
@arunmahadevan

arunmahadevan Dec 27, 2016

Contributor

+1

Contributor

arunmahadevan commented Dec 27, 2016

+1

@arunmahadevan

This comment has been minimized.

Show comment
Hide comment
@arunmahadevan

arunmahadevan Dec 30, 2016

Contributor

@aandis thanks for the work, merged to master and 1.x-branch. You can close the PR.

Contributor

arunmahadevan commented Dec 30, 2016

@aandis thanks for the work, merged to master and 1.x-branch. You can close the PR.

@aandis

This comment has been minimized.

Show comment
Hide comment
@aandis

aandis Dec 31, 2016

Contributor

Thanks @arunmahadevan. :)

Contributor

aandis commented Dec 31, 2016

Thanks @arunmahadevan. :)

@aandis aandis closed this Dec 31, 2016

@aandis aandis deleted the aandis:STORM-2203 branch Mar 6, 2017

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