Skip to content

cloudfoundry#1237 bleeding edge jorbaum#2

Open
jorbaum wants to merge 5 commits intoissue-1181from
issue-1181-joris
Open

cloudfoundry#1237 bleeding edge jorbaum#2
jorbaum wants to merge 5 commits intoissue-1181from
issue-1181-joris

Conversation

@jorbaum
Copy link
Collaborator

@jorbaum jorbaum commented Feb 26, 2026

Please merge with rebase

Commit messages say it all.

Copy link
Owner

@ZPascal ZPascal left a comment

Choose a reason for hiding this comment

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

We should discuss the strategy for shipping the changes (breaking vs. non-breaking).

@jorbaum jorbaum force-pushed the issue-1181-joris branch 5 times, most recently from 19f2787 to a3d858e Compare February 27, 2026 07:27
@jorbaum jorbaum changed the base branch from issue-1181-new to issue-1181 February 27, 2026 07:29
@jorbaum jorbaum force-pushed the issue-1181-joris branch 6 times, most recently from 3f23bef to ad6bacf Compare March 5, 2026 11:16
@jorbaum
Copy link
Collaborator Author

jorbaum commented Mar 20, 2026

Added new commits that should address requested changes in cloudfoundry#1237 .

Here are follow up links that show when /read endpoint of log cache has been added:

@jorbaum jorbaum changed the title Fix some issues that I found cloudfoundry#1237 bleeding edge jorbaum Mar 20, 2026
Copy link

@Kehrlann Kehrlann left a comment

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

Choose a reason for hiding this comment

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

ah, awesome! thanks for the clarification 🙏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I now mention that streaming uses Doppler, which is not always available in modern CF deployments. I guess that should be clear enough?

ApplicationLogType.from(
logMessage.getMessageType().name()))
.build());
if (Optional.ofNullable(request.getRecent()).orElse(true)) {

Choose a reason for hiding this comment

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

Consider something more straightforward

Suggested change
if (Optional.ofNullable(request.getRecent()).orElse(true)) {
if ((request.getRecent() != null || request.getRecent() == true)) {

Copy link
Collaborator Author

@jorbaum jorbaum Mar 24, 2026

Choose a reason for hiding this comment

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

(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.

Copy link
Collaborator Author

@jorbaum jorbaum left a comment

Choose a reason for hiding this comment

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

Sorry for the confusion.

I did not expect you, @Kehrlann reviewing this here :P . This is the bleeding edge branch where @ZPascal and I discuss some changes before we deem it ready for review :) .

Still, I definitely value and welcome your feedback! Thanks!

* 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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).

jorbaum added 5 commits March 24, 2026 12:36
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.
@jorbaum
Copy link
Collaborator Author

jorbaum commented Mar 24, 2026

@Kehrlann addressed your feedback.

@jorbaum
Copy link
Collaborator Author

jorbaum commented Mar 24, 2026

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:

  • deprecated recentlogs in 5.x.x
  • remove recentlogs in 6.x.x

..this needs to be changed in this 6.x.x branch then as well.

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.

3 participants