Skip to content

Comments

Remove redundant array creation#2129

Merged
sijie merged 5 commits intoapache:masterfrom
vzhikserg:remove-redundant-array-creation
Jul 29, 2019
Merged

Remove redundant array creation#2129
sijie merged 5 commits intoapache:masterfrom
vzhikserg:remove-redundant-array-creation

Conversation

@vzhikserg
Copy link
Contributor

@vzhikserg vzhikserg commented Jul 23, 2019

Changes

Remove redundant array creation in log statements to simplify the code.

eolivelli
eolivelli requested changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Cool.
I left a comment, please take a look

.../src/main/java/org/apache/distributedlog/exceptions/UnsupportedMetadataVersionException.java Outdated

public UnsupportedMetadataVersionException(String message) {
super(StatusCode.UNSUPPORTED_METADATA_VERSION, String.format(message));
super(StatusCode.UNSUPPORTED_METADATA_VERSION, message);
Copy link
Contributor

@eolivelli eolivelli Jul 23, 2019

Choose a reason for hiding this comment

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

I think this is a behavior change

Copy link
Contributor Author

@vzhikserg vzhikserg Jul 23, 2019

Choose a reason for hiding this comment

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

thanks for the comment. The change was reverted, but I don't see how this formatting can help without any arguments

Copy link
Contributor

@eolivelli eolivelli Jul 23, 2019

Choose a reason for hiding this comment

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

You are right, but it is better not to change things if not needed or out of the scope of the patch.
Sorry for so deep nitpicking :)

eolivelli
eolivelli approved these changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

Awesome work!

Let's wait for CI.
And I need some other committer to approve the patch

Cc @jiazhai

@vzhikserg
Copy link
Contributor Author

vzhikserg commented Jul 24, 2019

@eolivelli thanks! Do I need to investigate the results from CI or it is needed to rerun them?

@eolivelli
Copy link
Contributor

eolivelli commented Jul 25, 2019

Something is broken on CI, the presence of Maven 3.5.
We have to change things config.
I will fix it as soon as I can, @sijie do you have time to fix it?

@eolivelli
Copy link
Contributor

eolivelli commented Jul 27, 2019

@sijie I will send a PR soon for an upgrade of Maven to latest version

@eolivelli
Copy link
Contributor

eolivelli commented Jul 27, 2019

this is the fix for the upgrade of Maven #2135

@eolivelli
Copy link
Contributor

eolivelli commented Jul 27, 2019

rebuild java8

@eolivelli eolivelli mentioned this pull request Jul 27, 2019
Merged
@vzhikserg
Copy link
Contributor Author

vzhikserg commented Jul 28, 2019

@eolivelli thanks a lot for your help!

@eolivelli
Copy link
Contributor

eolivelli commented Jul 28, 2019

run pr validation

@eolivelli
Copy link
Contributor

eolivelli commented Jul 28, 2019

You have checkstyle violations:


2019-07-28T14:55:14.731 [INFO] --- maven-checkstyle-plugin:3.0.0:check (default-cli) @ bookkeeper-server ---
2019-07-28T14:55:36.573 [INFO] Starting audit...
[ERROR] /home/jenkins/jenkins-slave/workspace/bookkeeper_precommit_pullrequest_validation/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicy.java:2134: Line is longer than 120 characters (found 123). [LineLength]
[ERROR] /home/jenkins/jenkins-slave/workspace/bookkeeper_precommit_pullrequest_validation/bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestRackawareEnsemblePlacementPolicy.java:2142: Line is longer than 120 characters (found 123). [LineLength]

@eolivelli
Copy link
Contributor

eolivelli commented Jul 28, 2019

run this command to test your code for checkstyle

mvn checkstyle:check -Dstream

@vzhikserg
Copy link
Contributor Author

vzhikserg commented Jul 28, 2019

@eolivelli thanks, I don't know how I missed it, because right now I am adding the checkstyle validation to some pulsar modules ;)

@eolivelli eolivelli added this to the 4.10.0 milestone Jul 29, 2019
@sijie sijie merged commit 1df8ae9 into apache:master Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sijie sijie sijie approved these changes

@eolivelli eolivelli eolivelli approved these changes

Assignees

@vzhikserg vzhikserg

Labels

area/bookie area/client release/4.10.0 type/cleanup

Projects

None yet

Milestone

4.10.0

Development

Successfully merging this pull request may close these issues.

3 participants

@vzhikserg @eolivelli @sijie