Conversation
f40fbee to
da3e2c9
Compare
210376c to
b39e647
Compare
da3e2c9 to
b0f6345
Compare
ZPascal
left a comment
There was a problem hiding this comment.
We should discuss the strategy for shipping the changes (breaking vs. non-breaking).
...c/main/java/org/cloudfoundry/reactor/tokenprovider/_ClientCredentialsGrantTokenProvider.java
Show resolved
Hide resolved
...t-reactor/src/main/java/org/cloudfoundry/reactor/tokenprovider/AbstractUaaTokenProvider.java
Outdated
Show resolved
Hide resolved
...y-operations/src/main/java/org/cloudfoundry/operations/applications/DefaultApplications.java
Show resolved
Hide resolved
...erations/src/test/java/org/cloudfoundry/operations/applications/DefaultApplicationsTest.java
Outdated
Show resolved
Hide resolved
...y-operations/src/main/java/org/cloudfoundry/operations/applications/DefaultApplications.java
Show resolved
Hide resolved
cloudfoundry-client/src/main/java/org/cloudfoundry/doppler/DopplerClient.java
Outdated
Show resolved
Hide resolved
19f2787 to
a3d858e
Compare
3f23bef to
ad6bacf
Compare
ad6bacf to
b7800f7
Compare
|
Added new commits that should address requested changes in cloudfoundry#1237 . Here are follow up links that show when |
b7800f7 to
dd0ffb0
Compare
Kehrlann
left a comment
There was a problem hiding this comment.
Looks good, minor comments; the important bit is keeping @Deprecated but deprecated
| * List the applications logs. | ||
| * Only works with {@code Loggregator < 107.0}, shipped in {@code CFD < 24.3} | ||
| * and {@code TAS < 4.0}. | ||
| * List the applications logs. Uses Log Cache under the hood. |
There was a problem hiding this comment.
Please add a caveat that only "recent logs" use logcache, while streaming logs use Doppler and only work with Loggregator < 107.0 / CFC < 24.3
There was a problem hiding this comment.
AFAIU the "only work with Loggregator < 107.0 / CFC < 24.3" part is not accurate for streaming logs.
Loggregator v107.0.0 only removed the RecentLogsHandler from Traffic Controller - not Doppler or the streaming endpoints. The Doppler instance group is still present in the cf-deployment manifest at both v24.3.0 and the latest v55.0.0.
My suggestion would be to add a note clarifying that only recent logs use LogCache, while streaming logs still use Doppler (which remains available on current CF deployments just not in the new Shared Nothing Architecture).
There was a problem hiding this comment.
ah, awesome! thanks for the clarification 🙏
There was a problem hiding this comment.
I now mention that streaming uses Doppler, which is not always available in modern CF deployments. I guess that should be clear enough?
...dfoundry-operations/src/main/java/org/cloudfoundry/operations/applications/Applications.java
Show resolved
Hide resolved
| ApplicationLogType.from( | ||
| logMessage.getMessageType().name())) | ||
| .build()); | ||
| if (Optional.ofNullable(request.getRecent()).orElse(true)) { |
There was a problem hiding this comment.
Consider something more straightforward
| if (Optional.ofNullable(request.getRecent()).orElse(true)) { | |
| if ((request.getRecent() != null || request.getRecent() == true)) { |
There was a problem hiding this comment.
(request.getRecent() != null || request.getRecent() == true) would enter the if-branch when recent is false too, since != null is true.
I therefore used instead:
if (request.getRecent() == null || request.getRecent()) {However, I copy + pasted that syntax from getLogs():
private static Flux<LogMessage> getLogs(
Mono<DopplerClient> dopplerClient, String applicationId, Boolean recent) {
if (Optional.ofNullable(recent).orElse(false)) {
...I could change it there as well.
...dfoundry-operations/src/main/java/org/cloudfoundry/operations/applications/Applications.java
Show resolved
Hide resolved
| * List the applications logs. | ||
| * Only works with {@code Loggregator < 107.0}, shipped in {@code CFD < 24.3} | ||
| * and {@code TAS < 4.0}. | ||
| * List the applications logs. Uses Log Cache under the hood. |
There was a problem hiding this comment.
AFAIU the "only work with Loggregator < 107.0 / CFC < 24.3" part is not accurate for streaming logs.
Loggregator v107.0.0 only removed the RecentLogsHandler from Traffic Controller - not Doppler or the streaming endpoints. The Doppler instance group is still present in the cf-deployment manifest at both v24.3.0 and the latest v55.0.0.
My suggestion would be to add a note clarifying that only recent logs use LogCache, while streaming logs still use Doppler (which remains available on current CF deployments just not in the new Shared Nothing Architecture).
Use LogCacheClient.read() for recent logs (the default), fall back to Doppler streaming when recent is explicitly false. Remove logCacheLogs() integration test — it was a temporary reference for the direct LogCache API, now redundant since logs() exercises the same path. Remove logsRecent() and its helpers likewise.
LogCache has been available since cf-deployment v3.0.0 (July 2018). Lower the version gate from PCF_4_v2 to PCF_2_3 and update the javadoc to reflect that the test now exercises the LogCache-backed path.
logs(ApplicationLogsRequest) now uses Log Cache internally, so Operations API users do not need to migrate.
dd0ffb0 to
f681879
Compare
|
@Kehrlann addressed your feedback. |
|
I also rebased this branch on top of 5.x.x in the following branch - https://github.com/ZPascal/cf-java-client/tree/issue-1181-5.x.x-joris . The idea was to:
..this needs to be changed in this 6.x.x branch then as well. |
Please merge with rebase
Commit messages say it all.