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

STORM-3456: cassandra: fix all checkstyle warnings #3074

Merged
merged 1 commit into from
Jul 6, 2019
Merged

STORM-3456: cassandra: fix all checkstyle warnings #3074

merged 1 commit into from
Jul 6, 2019

Conversation

krichter722
Copy link
Contributor

No description provided.

@@ -99,6 +98,7 @@ public Values toValues(OpaqueValue<ITuple> tuple) {
prev = null;
}

Long currTx = (Long) values.get(index++);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this introduces a bug. The index is supposed to be 0 here, but the for loops above can increment it. Maybe initialize index to 1 above, and then just do get(0) here.

@@ -87,6 +86,7 @@ public Values toValues(TransactionalValue<ITuple> tuple) {
curr = null;
}

Long txId = (Long) values.get(index++);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same issue here

@srdo
Copy link
Contributor

srdo commented Jul 6, 2019

+1.

Please fix comments in a new commit on future PRs, Github's force push diffs aren't very good yet (e.g https://github.com/apache/storm/compare/f4698976dc16f6c6303f8877ef82418ec42c8c57..e7f2c919a1a99621450a945e7ad473c0d8641a9a contains a bunch of changes outside this module), so it's hard to tell exactly what changed. It's faster to review only the new changes rather than looking at the full diff again. Thanks.

@srdo srdo merged commit 8cd3a53 into apache:master Jul 6, 2019
@krichter722 krichter722 deleted the checkstyle-cassandra branch August 23, 2019 07:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants