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.

Sign up for GitHub

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

Jump to bottom

[improve][admin][broker] Admin API: stream internal topic stats #22510

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[improve][admin][broker] Admin API: stream internal topic stats #22510

wants to merge 2 commits into from

Conversation

eolivelli
Copy link
Contributor

@eolivelli eolivelli commented Apr 15, 2024 •
edited
Loading

Motivation

The JSON may be huge when individuallyDeletedMessages inside ManagedLedgerInternalStats$CursorStats contains many ranges

Modifications

Instead of returning the Object and letting the framework do the encoding, switch to StreamingOutput and perform the JSON encoding manually.
One step further may be to start serving the results as soon as they are available, instead of build the object in memory and then perform the encoding, but this is far more involved

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: eolivelli#25

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 15, 2024
@eolivelli eolivelli changed the title Admin API: stream stats Admin API: stream internal topic stats Apr 15, 2024
@eolivelli eolivelli mentioned this pull request Apr 15, 2024
Open
@lhotari
Copy link
Member

lhotari commented Apr 15, 2024 •
edited
Loading

@eolivelli have you had a chance to check if Gzip compression helps with large responses? Pulsar admin client changes: #22464, broker side changes: #21667, #22463 and #22370 .

@lhotari
Copy link
Member

lhotari commented Apr 15, 2024

The JSON may be huge when individuallyDeletedMessages inside ManagedLedgerInternalStats$CursorStats contains many ranges

@eolivelli do you have any figures to share? How large is the JSON response? How many individuallyDeletedMessages are there?

lhotari
lhotari reviewed Apr 15, 2024
View reviewed changes
pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/PersistentTopics.java Outdated
Comment on lines 1242 to 1243
@Context HttpServletRequest servletRequest;

Copy link
Member

Choose a reason for hiding this comment

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

Is this needed in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a left over, sorry

@eolivelli
Copy link
Contributor Author

@eolivelli have you had a chance to check if Gzip compression helps with large responses? Pulsar admin client changes: #22464, broker side changes: #21667, #22463 and #22370 .

I have seen those PRs, great stuff, but the problem here is about the serialization happening in some upgredictable place, like this:


"pulsar-web-48-18" #174 prio=5 os_prio=0 cpu=1421.36ms elapsed=456.08s tid=0x00007f6a54002260 nid=0x342c0 waiting on condition  [0x00007f6e5c951000]
   java.lang.Thread.State: WAITING (parking)
        at jdk.internal.misc.Unsafe.park(java.base@17.0.9/Native Method)
        - parking to wait for  <0x00001007fe5a5b38> (a java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject)
        at java.util.concurrent.locks.LockSupport.park(java.base@17.0.9/LockSupport.java:341)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionNode.block(java.base@17.0.9/AbstractQueuedSynchronizer.java:506)
        at java.util.concurrent.ForkJoinPool.unmanagedBlock(java.base@17.0.9/ForkJoinPool.java:3465)
        at java.util.concurrent.ForkJoinPool.managedBlock(java.base@17.0.9/ForkJoinPool.java:3436)
        at java.util.concurrent.locks.AbstractQueuedSynchronizer$ConditionObject.await(java.base@17.0.9/AbstractQueuedSynchronizer.java:1623)
        at org.eclipse.jetty.util.SharedBlockingCallback$Blocker.block(SharedBlockingCallback.java:241)
        at org.eclipse.jetty.server.HttpOutput.channelWrite(HttpOutput.java:270)
        at org.eclipse.jetty.server.HttpOutput.write(HttpOutput.java:873)
        at org.glassfish.jersey.servlet.internal.ResponseWriter$NonCloseableOutputStreamWrapper.write(ResponseWriter.java:301)
        at org.glassfish.jersey.message.internal.CommittingOutputStream.write(CommittingOutputStream.java:200)
        at org.glassfish.jersey.message.internal.WriterInterceptorExecutor$UnCloseableOutputStream.write(WriterInterceptorExecutor.java:276)
        at com.fasterxml.jackson.core.json.UTF8JsonGenerator._flushBuffer(UTF8JsonGenerator.java:2203)
        at com.fasterxml.jackson.core.json.UTF8JsonGenerator._writeStringSegments(UTF8JsonGenerator.java:1343)
        at com.fasterxml.jackson.core.json.UTF8JsonGenerator.writeString(UTF8JsonGenerator.java:517)
        at com.fasterxml.jackson.databind.ser.std.StringSerializer.serialize(StringSerializer.java:41)
        at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:733)
        at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:774)
        at com.fasterxml.jackson.databind.ser.BeanSerializer.serialize(BeanSerializer.java:178)
        at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeOptionalFields(MapSerializer.java:869)
        at com.fasterxml.jackson.databind.ser.std.MapSerializer.serializeWithoutTypeInfo(MapSerializer.java:760)
        at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:720)
        at com.fasterxml.jackson.databind.ser.std.MapSerializer.serialize(MapSerializer.java:35)
        at com.fasterxml.jackson.databind.ser.BeanPropertyWriter.serializeAsField(BeanPropertyWriter.java:733)
        at com.fasterxml.jackson.databind.ser.std.BeanSerializerBase.serializeFields(BeanSerializerBase.java:774)
        at com.fasterxml.jackson.databind.ser.BeanSerializer.serialize(BeanSerializer.java:178)
        at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider._serialize(DefaultSerializerProvider.java:480)
        at com.fasterxml.jackson.databind.ser.DefaultSerializerProvider.serializeValue(DefaultSerializerProvider.java:319)
        at com.fasterxml.jackson.databind.ObjectWriter$Prefetch.serialize(ObjectWriter.java:1572)
        at com.fasterxml.jackson.databind.ObjectWriter.writeValue(ObjectWriter.java:1061)
        at org.glassfish.jersey.jackson.internal.jackson.jaxrs.base.ProviderBase.writeTo(ProviderBase.java:647)
        at org.glassfish.jersey.message.internal.WriterInterceptorExecutor$TerminalWriterInterceptor.invokeWriteTo(WriterInterceptorExecutor.java:242)
        at org.glassfish.jersey.message.internal.WriterInterceptorExecutor$TerminalWriterInterceptor.aroundWriteTo(WriterInterceptorExecutor.java:227)
        at org.glassfish.jersey.message.internal.WriterInterceptorExecutor.proceed(WriterInterceptorExecutor.java:139)
        at org.glassfish.jersey.server.internal.JsonWithPaddingInterceptor.aroundWriteTo(JsonWithPaddingInterceptor.java:85)
        at org.glassfish.jersey.message.internal.WriterInterceptorExecutor.proceed(WriterInterceptorExecutor.java:139)
        at org.glassfish.jersey.server.internal.MappableExceptionWrapperInterceptor.aroundWriteTo(MappableExceptionWrapperInterceptor.java:61)
        at org.glassfish.jersey.message.internal.WriterInterceptorExecutor.proceed(WriterInterceptorExecutor.java:139)
        at org.glassfish.jersey.message.internal.MessageBodyFactory.writeTo(MessageBodyFactory.java:1116)
        at org.glassfish.jersey.server.ServerRuntime$Responder.writeResponse(ServerRuntime.java:635)
        at org.glassfish.jersey.server.ServerRuntime$Responder.processResponse(ServerRuntime.java:373)
        at org.glassfish.jersey.server.ServerRuntime$Responder.process(ServerRuntime.java:363)
        at org.glassfish.jersey.server.ServerRuntime$AsyncResponder$3.run(ServerRuntime.java:857)

@eolivelli
Copy link
Contributor Author

How many individuallyDeletedMessages are there?
more than 100.000

@lhotari
Copy link
Member

lhotari commented Apr 15, 2024

I have seen those PRs, great stuff, but the problem here is about the serialization happening in some upgredictable place, like this:

Trying to understand this more deeper. What is the impact of using the default serialization in this case? how does the StreamingOutput based serialization improve the situation?

@dao-jun
Copy link
Member

dao-jun commented Apr 15, 2024

I have seen those PRs, great stuff, but the problem here is about the serialization happening in some upgredictable place, like this:

Trying to understand this more deeper. What is the impact of using the default serialization in this case? how does the StreamingOutput based serialization improve the situation?

@lhotari I guess the default implementation of the jetty is

String jsonString = JSON.toJsonString(object);
httpOutput.writeString(jsonString);

It will generate the jsonString first and then send it to the client. But by this PR, it doesn't need to generate the jsonString first in the memory, but sends the result to the client directly. So we can reduce heap memory usage and avoid more memory copies. @eolivelli Am I right?
I support the PR, it could help a lot if the response body is huge.

@dao-jun dao-jun changed the title Admin API: stream internal topic stats [improve][admin][broker] Admin API: stream internal topic stats Apr 15, 2024
@dao-jun dao-jun closed this Apr 15, 2024
@dao-jun dao-jun reopened this Apr 15, 2024
@lhotari
Copy link
Member

lhotari commented Apr 16, 2024 •
edited
Loading 

String jsonString = JSON.toJsonString(object);
httpOutput.writeString(jsonString);

@dao-jun this doesn't seem to be the case based on the stack trace that Enrico shared. The default solution even without this PR is "streaming". Based on the stack trace, it's using this class to create the JSON response: https://github.com/eclipse-ee4j/jersey/blob/2.41/media/json-jackson/src/main/java/org/glassfish/jersey/jackson/internal/jackson/jaxrs/base/ProviderBase.java#L583-L659

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lhotari lhotari lhotari left review comments

@Technoboy- Technoboy- Awaiting requested review from Technoboy-

@mattisonchao mattisonchao Awaiting requested review from mattisonchao

@poorbarcode poorbarcode Awaiting requested review from poorbarcode

At least 1 approving review is required to merge this pull request.

Assignees

@eolivelli eolivelli

Labels
area/admin area/broker category/performance Performance issues fix or improvements doc-not-needed Your PR changes do not impact docs ready-to-test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eolivelli @lhotari @dao-jun