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

[Minor] Fix Variable initialization #602

Merged
merged 2 commits into from
Jan 19, 2018

Conversation

satybald
Copy link
Contributor

@satybald satybald commented Jan 7, 2018

Combine variable declartion and intiaztiona together.

@satybald
Copy link
Contributor Author

satybald commented Jan 8, 2018

I'm wondering what do the team think about enabling style checks plugin? I noticed that KSQL inherits from confluent common, where the code style is defined. But code style issues de not fail the build.

@satybald
Copy link
Contributor Author

For some reason, commit fails and I cannot see why. Would it be possible to share the result of the build? @hjafarpour @miguno @dguy @bluemonk3y

@bluemonk3y
Copy link

retest this pls

@dguy
Copy link
Contributor

dguy commented Jan 11, 2018

Checkstyle is, or at least was enabled and should fail the build. However, i'm not sure which checkstyle rules it is using at this stage as there is clearly code that should fail the checks. Will need to investigate further

String udafName = getFunctionList().get(aggFunctionVarSuffix).getName()
.getSuffix();
KsqlAggregateFunction aggregateFunction = functionRegistry.getAggregateFunction(udafName,
getFunctionList()
.get(aggFunctionVarSuffix).getArguments(), schema);
fieldSchema = aggregateFunction.getReturnType();
Schema fieldSchema = aggregateFunction.getReturnType();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably take this one step further and just inline it. There is no reason for having the variable

@dguy
Copy link
Contributor

dguy commented Jan 12, 2018

retest this please

Copy link
Contributor

@dguy dguy left a comment

Choose a reason for hiding this comment

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

Thanks @satybald, LGTM

@satybald
Copy link
Contributor Author

my pleasure @dguy. Wondering when this patch can be merged?

Copy link
Contributor

@hjafarpour hjafarpour left a comment

Choose a reason for hiding this comment

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

LGTM.

@bluemonk3y
Copy link

jenkins ran out of disk

@bluemonk3y
Copy link

retest this please

@satybald
Copy link
Contributor Author

@bluemonk3y thanks for the information. What're your thoughts about having a public Travis CI build system? You can still have a private confluent Jenkins and ran all internal integration tests there.

@dguy
Copy link
Contributor

dguy commented Jan 18, 2018

retest this please

@satybald
Copy link
Contributor Author

@dguy @bluemonk3y @hjafarpour locally it compiles without any problem. However, the branch contains flaky integration tests that fails the build.
compilation

@dguy
Copy link
Contributor

dguy commented Jan 18, 2018

It is failing because there is no space left on the disk jenkins is using. Unfortunately we can't merge until we get a clean build. Sorry for the wait @satybald

@satybald
Copy link
Contributor Author

@dguy I tested it locally. I noticed that integration tests in serde module do not pass.

@bluemonk3y
Copy link

retest this please

@dguy
Copy link
Contributor

dguy commented Jan 19, 2018

Build finally passed. Thanks for your patience @satybald

@dguy dguy merged commit 43b2037 into confluentinc:master Jan 19, 2018
@satybald satybald deleted the fix-variable-initialization branch January 19, 2018 18:47
@satybald
Copy link
Contributor Author

yea 🎉

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.

4 participants