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

[CELEBORN-1390] ServletContextHandler should allow null path info to avoid redirection #2464

Closed
wants to merge 1 commit into from
Closed

[CELEBORN-1390] ServletContextHandler should allow null path info to avoid redirection #2464

wants to merge 1 commit into from

Conversation

SteNicholas
Copy link
Member

What changes were proposed in this pull request?

ServletContextHandler allows null path info to avoid redirection via setAllowNullPathInfo(true).

Why are the changes needed?

ServletContextHandler does not allow null path info which causes that celeborn.metrics.prometheus.path and celeborn.metrics.json.path could not access without redirection. For example:

celeborn@celeborn-test:/data/service/celeborn$ curl http://localhost:9096/metrics/prometheus
celeborn@celeborn-test:/data/service/celeborn$ curl http://localhost:9096/metrics/prometheus/
metrics_WriteDataHardSplitCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataWriteFailCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataConnectionExceptionCount_Count{role="Worker"} 0 1713182689795
metrics_FetchChunkFailCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataCreateConnectionFailCount_Count{role="Worker"} 0 1713182689795
metrics_WriteDataSuccessCount_Count{role="Worker"} 0 1713182689795
metrics_FetchChunkSuccessCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataFailCount_Count{role="Worker"} 0 1713182689795
metrics_RegionStartFailCount_Count{role="Worker"} 0 1713182689795
metrics_RegionFinishFailCount_Count{role="Worker"} 0 1713182689795
metrics_ActiveConnectionCount_Count{role="Worker"} 0 1713182689795
metrics_SlotsAllocated_Count{role="Worker"} 0 1713182689795
metrics_OpenStreamSuccessCount_Count{role="Worker"} 0 1713182689795
metrics_WriteDataFailCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataFailNonCriticalCauseCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataTimeoutCount_Count{role="Worker"} 0 1713182689795
metrics_PushDataHandshakeFailCount_Count{role="Worker"} 0 1713182689795
metrics_OpenStreamFailCount_Count{role="Worker"} 0 1713182689795

ServletContextHandler should allow null path info to avoid redirection via setAllowNullPathInfo(true). setAllowNullPathInfo() sets true if /context is not redirected to /context/.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • ApiMasterResourceSuite
  • ApiWorkerResourceSuite
celeborn@celeborn-test:/data/service/celeborn$ curl http://localhost:9096/metrics/prometheus
metrics_WriteDataHardSplitCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataWriteFailCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataConnectionExceptionCount_Count{role="Worker"} 0 1713182689795
metrics_FetchChunkFailCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataCreateConnectionFailCount_Count{role="Worker"} 0 1713182689795
metrics_WriteDataSuccessCount_Count{role="Worker"} 0 1713182689795
metrics_FetchChunkSuccessCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataFailCount_Count{role="Worker"} 0 1713182689795
metrics_RegionStartFailCount_Count{role="Worker"} 0 1713182689795
metrics_RegionFinishFailCount_Count{role="Worker"} 0 1713182689795
metrics_ActiveConnectionCount_Count{role="Worker"} 0 1713182689795
metrics_SlotsAllocated_Count{role="Worker"} 0 1713182689795
metrics_OpenStreamSuccessCount_Count{role="Worker"} 0 1713182689795
metrics_WriteDataFailCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataFailNonCriticalCauseCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataTimeoutCount_Count{role="Worker"} 0 1713182689795
metrics_PushDataHandshakeFailCount_Count{role="Worker"} 0 1713182689795
metrics_OpenStreamFailCount_Count{role="Worker"} 0 1713182689795
celeborn@celeborn-test:/data/service/celeborn$ curl http://localhost:9096/metrics/prometheus/
metrics_WriteDataHardSplitCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataWriteFailCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataConnectionExceptionCount_Count{role="Worker"} 0 1713182689795
metrics_FetchChunkFailCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataCreateConnectionFailCount_Count{role="Worker"} 0 1713182689795
metrics_WriteDataSuccessCount_Count{role="Worker"} 0 1713182689795
metrics_FetchChunkSuccessCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataFailCount_Count{role="Worker"} 0 1713182689795
metrics_RegionStartFailCount_Count{role="Worker"} 0 1713182689795
metrics_RegionFinishFailCount_Count{role="Worker"} 0 1713182689795
metrics_ActiveConnectionCount_Count{role="Worker"} 0 1713182689795
metrics_SlotsAllocated_Count{role="Worker"} 0 1713182689795
metrics_OpenStreamSuccessCount_Count{role="Worker"} 0 1713182689795
metrics_WriteDataFailCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataFailNonCriticalCauseCount_Count{role="Worker"} 0 1713182689795
metrics_ReplicateDataTimeoutCount_Count{role="Worker"} 0 1713182689795
metrics_PushDataHandshakeFailCount_Count{role="Worker"} 0 1713182689795
metrics_OpenStreamFailCount_Count{role="Worker"} 0 1713182689795

@SteNicholas
Copy link
Member Author

Ping @RexXiong, @turboFei.

RexXiong
RexXiong approved these changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@RexXiong RexXiong left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

turboFei
turboFei approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@turboFei turboFei left a comment

Choose a reason for hiding this comment

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

thanks

@RexXiong
Copy link
Contributor

Thanks, merge to main(v0.5.0)

@RexXiong RexXiong closed this in 8c65ddd Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RexXiong RexXiong RexXiong approved these changes

@turboFei turboFei turboFei approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SteNicholas @RexXiong @turboFei