You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Kevin Sweeney <ke...@apache.org> on 2015/07/22 21:30:34 UTC
Review Request 36703: Remove unnecessary uses of Guava Joiner.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36703/
-----------------------------------------------------------
Review request for Aurora, Joshua Cohen and Bill Farner.
Repository: aurora
Description
-------
Remove unnecessary use of Guava Joiner.
This replaces a Guava-ism with the JDK8 standard library APIs. Most of the changes to use `String::join` instead of `Joiner.on(...).join(...)` should be noncontroversial; however in some cases the resulting code is more verbose. This is mostly due to the fact that the standard library requires that arguments be instances of `CharSequence`, requiring the caller to map over `Object::toString`. I'd argue minimizing use of Guava-isms when a standard lib alternative exists is preferable in these cases but don't feel strongly about it.
There are also some opportunistic changes to surrounding code to use lambdas, method references, and the standard `Streams` API instead of `FluentIterable` and `Ordering`.
`Joiner` is still used in its `withKeyValueSeparator` form, as no standard library alternative exists for it.
Diffs
-----
src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java 2b7613741a729e7065bbe74690d543b45802c400
src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java b4163435ea337a9976fae2f84850af0320ab9884
src/main/java/org/apache/aurora/scheduler/base/JobKeys.java a5ffa5e95b301e536a84acf02817ea0c080559d0
src/main/java/org/apache/aurora/scheduler/cron/CrontabEntry.java 904dd72dbb9036d8c06353a953bf35b8f04cfdbc
src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java 45e062d3acbd7b8565bd3773c1e994aae96378e0
src/main/java/org/apache/aurora/scheduler/http/CorsFilter.java e413ad9bdccc329777b3d0764ba8684539956679
src/main/java/org/apache/aurora/scheduler/http/Slaves.java b64e18c4d5278e6cdb2e480043a45e6ec4f87484
src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 63c31eeb293771b6cc0a3e4cc62dd3a94853e727
src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroIniParser.java ff8063c050e13b4bffda2661a817fdc023b80867
src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java 7232f603e21bbc9dbb5e05aedd5e493de519158e
src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 92c970c34ca9dc4f052760e5a3d3770a089d9a67
src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 7c8f8b9b8d7deb082edc0f85a6d3da1536735545
Diff: https://reviews.apache.org/r/36703/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Kevin Sweeney
Re: Review Request 36703: Replace Guava Joiner with String::join where
simpler
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36703/#review92662
-----------------------------------------------------------
Master (8bdfb85) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On July 22, 2015, 10:08 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36703/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 10:08 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Now that `String::join` is part of the standard library some uses of Guava's `Joiner` can be removed.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/base/JobKeys.java a5ffa5e95b301e536a84acf02817ea0c080559d0
> src/main/java/org/apache/aurora/scheduler/cron/CrontabEntry.java 904dd72dbb9036d8c06353a953bf35b8f04cfdbc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java 45e062d3acbd7b8565bd3773c1e994aae96378e0
> src/main/java/org/apache/aurora/scheduler/http/CorsFilter.java e413ad9bdccc329777b3d0764ba8684539956679
> src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 7c8f8b9b8d7deb082edc0f85a6d3da1536735545
>
> Diff: https://reviews.apache.org/r/36703/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 36703: Replace Guava Joiner with String::join where
simpler
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36703/#review92669
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Farner
On July 22, 2015, 10:08 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36703/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 10:08 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Now that `String::join` is part of the standard library some uses of Guava's `Joiner` can be removed.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/base/JobKeys.java a5ffa5e95b301e536a84acf02817ea0c080559d0
> src/main/java/org/apache/aurora/scheduler/cron/CrontabEntry.java 904dd72dbb9036d8c06353a953bf35b8f04cfdbc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java 45e062d3acbd7b8565bd3773c1e994aae96378e0
> src/main/java/org/apache/aurora/scheduler/http/CorsFilter.java e413ad9bdccc329777b3d0764ba8684539956679
> src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 7c8f8b9b8d7deb082edc0f85a6d3da1536735545
>
> Diff: https://reviews.apache.org/r/36703/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 36703: Replace Guava Joiner with String::join where
simpler
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36703/
-----------------------------------------------------------
(Updated July 22, 2015, 3:08 p.m.)
Review request for Aurora, Joshua Cohen and Bill Farner.
Changes
-------
Review feedback.
Summary (updated)
-----------------
Replace Guava Joiner with String::join where simpler
Repository: aurora
Description (updated)
-------
Now that `String::join` is part of the standard library some uses of Guava's `Joiner` can be removed.
Diffs (updated)
-----
src/main/java/org/apache/aurora/scheduler/base/JobKeys.java a5ffa5e95b301e536a84acf02817ea0c080559d0
src/main/java/org/apache/aurora/scheduler/cron/CrontabEntry.java 904dd72dbb9036d8c06353a953bf35b8f04cfdbc
src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java 45e062d3acbd7b8565bd3773c1e994aae96378e0
src/main/java/org/apache/aurora/scheduler/http/CorsFilter.java e413ad9bdccc329777b3d0764ba8684539956679
src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 7c8f8b9b8d7deb082edc0f85a6d3da1536735545
Diff: https://reviews.apache.org/r/36703/diff/
Testing
-------
./gradlew -Pq build
Thanks,
Kevin Sweeney
Re: Review Request 36703: Remove unnecessary uses of Guava Joiner.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36703/#review92637
-----------------------------------------------------------
Master (6e2bf57) is green with this patch.
./build-support/jenkins/build.sh
However, it appears that it might lack test coverage.
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On July 22, 2015, 7:30 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36703/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 7:30 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Remove unnecessary use of Guava Joiner.
>
> This replaces a Guava-ism with the JDK8 standard library APIs. Most of the changes to use `String::join` instead of `Joiner.on(...).join(...)` should be noncontroversial; however in some cases the resulting code is more verbose. This is mostly due to the fact that the standard library requires that arguments be instances of `CharSequence`, requiring the caller to map over `Object::toString`. I'd argue minimizing use of Guava-isms when a standard lib alternative exists is preferable in these cases but don't feel strongly about it.
>
> There are also some opportunistic changes to surrounding code to use lambdas, method references, and the standard `Streams` API instead of `FluentIterable` and `Ordering`.
>
> `Joiner` is still used in its `withKeyValueSeparator` form, as no standard library alternative exists for it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java 2b7613741a729e7065bbe74690d543b45802c400
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java b4163435ea337a9976fae2f84850af0320ab9884
> src/main/java/org/apache/aurora/scheduler/base/JobKeys.java a5ffa5e95b301e536a84acf02817ea0c080559d0
> src/main/java/org/apache/aurora/scheduler/cron/CrontabEntry.java 904dd72dbb9036d8c06353a953bf35b8f04cfdbc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java 45e062d3acbd7b8565bd3773c1e994aae96378e0
> src/main/java/org/apache/aurora/scheduler/http/CorsFilter.java e413ad9bdccc329777b3d0764ba8684539956679
> src/main/java/org/apache/aurora/scheduler/http/Slaves.java b64e18c4d5278e6cdb2e480043a45e6ec4f87484
> src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 63c31eeb293771b6cc0a3e4cc62dd3a94853e727
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroIniParser.java ff8063c050e13b4bffda2661a817fdc023b80867
> src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java 7232f603e21bbc9dbb5e05aedd5e493de519158e
> src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 92c970c34ca9dc4f052760e5a3d3770a089d9a67
> src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 7c8f8b9b8d7deb082edc0f85a6d3da1536735545
>
> Diff: https://reviews.apache.org/r/36703/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 36703: Remove unnecessary uses of Guava Joiner.
Posted by Joshua Cohen <jc...@apache.org>.
> On July 22, 2015, 7:36 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java, line 45
> > <https://reviews.apache.org/r/36703/diff/1/?file=1019008#file1019008line45>
> >
> > Yeah, big -1 to me on changes like this. The previous code was clear and concise. The patch is a readability regression IMHO with no obvious upside.
+1 to this -1. The straight Joiner.on -> String.join changes are great, but the other changes really obfuscate what's going on.
- Joshua
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36703/#review92635
-----------------------------------------------------------
On July 22, 2015, 7:30 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36703/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 7:30 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Remove unnecessary use of Guava Joiner.
>
> This replaces a Guava-ism with the JDK8 standard library APIs. Most of the changes to use `String::join` instead of `Joiner.on(...).join(...)` should be noncontroversial; however in some cases the resulting code is more verbose. This is mostly due to the fact that the standard library requires that arguments be instances of `CharSequence`, requiring the caller to map over `Object::toString`. I'd argue minimizing use of Guava-isms when a standard lib alternative exists is preferable in these cases but don't feel strongly about it.
>
> There are also some opportunistic changes to surrounding code to use lambdas, method references, and the standard `Streams` API instead of `FluentIterable` and `Ordering`.
>
> `Joiner` is still used in its `withKeyValueSeparator` form, as no standard library alternative exists for it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java 2b7613741a729e7065bbe74690d543b45802c400
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java b4163435ea337a9976fae2f84850af0320ab9884
> src/main/java/org/apache/aurora/scheduler/base/JobKeys.java a5ffa5e95b301e536a84acf02817ea0c080559d0
> src/main/java/org/apache/aurora/scheduler/cron/CrontabEntry.java 904dd72dbb9036d8c06353a953bf35b8f04cfdbc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java 45e062d3acbd7b8565bd3773c1e994aae96378e0
> src/main/java/org/apache/aurora/scheduler/http/CorsFilter.java e413ad9bdccc329777b3d0764ba8684539956679
> src/main/java/org/apache/aurora/scheduler/http/Slaves.java b64e18c4d5278e6cdb2e480043a45e6ec4f87484
> src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 63c31eeb293771b6cc0a3e4cc62dd3a94853e727
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroIniParser.java ff8063c050e13b4bffda2661a817fdc023b80867
> src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java 7232f603e21bbc9dbb5e05aedd5e493de519158e
> src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 92c970c34ca9dc4f052760e5a3d3770a089d9a67
> src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 7c8f8b9b8d7deb082edc0f85a6d3da1536735545
>
> Diff: https://reviews.apache.org/r/36703/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Kevin Sweeney
>
>
Re: Review Request 36703: Remove unnecessary uses of Guava Joiner.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36703/#review92635
-----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java (line 45)
<https://reviews.apache.org/r/36703/#comment146858>
Yeah, big -1 to me on changes like this. The previous code was clear and concise. The patch is a readability regression IMHO with no obvious upside.
- Bill Farner
On July 22, 2015, 7:30 p.m., Kevin Sweeney wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36703/
> -----------------------------------------------------------
>
> (Updated July 22, 2015, 7:30 p.m.)
>
>
> Review request for Aurora, Joshua Cohen and Bill Farner.
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Remove unnecessary use of Guava Joiner.
>
> This replaces a Guava-ism with the JDK8 standard library APIs. Most of the changes to use `String::join` instead of `Joiner.on(...).join(...)` should be noncontroversial; however in some cases the resulting code is more verbose. This is mostly due to the fact that the standard library requires that arguments be instances of `CharSequence`, requiring the caller to map over `Object::toString`. I'd argue minimizing use of Guava-isms when a standard lib alternative exists is preferable in these cases but don't feel strongly about it.
>
> There are also some opportunistic changes to surrounding code to use lambdas, method references, and the standard `Streams` API instead of `FluentIterable` and `Ordering`.
>
> `Joiner` is still used in its `withKeyValueSeparator` form, as no standard library alternative exists for it.
>
>
> Diffs
> -----
>
> src/main/java/org/apache/aurora/scheduler/app/VolumeParser.java 2b7613741a729e7065bbe74690d543b45802c400
> src/main/java/org/apache/aurora/scheduler/async/JobUpdateHistoryPruner.java b4163435ea337a9976fae2f84850af0320ab9884
> src/main/java/org/apache/aurora/scheduler/base/JobKeys.java a5ffa5e95b301e536a84acf02817ea0c080559d0
> src/main/java/org/apache/aurora/scheduler/cron/CrontabEntry.java 904dd72dbb9036d8c06353a953bf35b8f04cfdbc
> src/main/java/org/apache/aurora/scheduler/cron/quartz/Quartz.java 45e062d3acbd7b8565bd3773c1e994aae96378e0
> src/main/java/org/apache/aurora/scheduler/http/CorsFilter.java e413ad9bdccc329777b3d0764ba8684539956679
> src/main/java/org/apache/aurora/scheduler/http/Slaves.java b64e18c4d5278e6cdb2e480043a45e6ec4f87484
> src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java 63c31eeb293771b6cc0a3e4cc62dd3a94853e727
> src/main/java/org/apache/aurora/scheduler/http/api/security/ShiroIniParser.java ff8063c050e13b4bffda2661a817fdc023b80867
> src/main/java/org/apache/aurora/scheduler/log/mesos/MesosLogStreamModule.java 7232f603e21bbc9dbb5e05aedd5e493de519158e
> src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 92c970c34ca9dc4f052760e5a3d3770a089d9a67
> src/main/java/org/apache/aurora/scheduler/thrift/aop/LoggingInterceptor.java 7c8f8b9b8d7deb082edc0f85a6d3da1536735545
>
> Diff: https://reviews.apache.org/r/36703/diff/
>
>
> Testing
> -------
>
> ./gradlew -Pq build
>
>
> Thanks,
>
> Kevin Sweeney
>
>