github-actions[bot] commented on code in PR #60921:
URL: https://github.com/apache/doris/pull/60921#discussion_r3379302988
##########
fe/fe-core/src/main/java/org/apache/doris/system/HeartbeatMgr.java:
##########
@@ -96,7 +97,7 @@ public void setMaster(int clusterId, String token, long
epoch) {
TMasterInfo tMasterInfo = new TMasterInfo(
new TNetworkAddress(FrontendOptions.getLocalHostAddress(),
Config.rpc_port), clusterId, epoch);
tMasterInfo.setToken(token);
- tMasterInfo.setHttpPort(Config.http_port);
+ tMasterInfo.setHttpPort(HttpURLUtil.getHttpPort());
long flags = heartbeatFlags.getHeartbeatFlags();
tMasterInfo.setHeartbeatFlags(flags);
Review Comment:
Sending `https_port` here breaks the current BE consumer of
`TMasterInfo.http_port`. BE stores this as `ClusterInfo::master_fe_http_port`
in `heartbeat_server.cpp`, and `SmallFileMgr::_download_file()` builds
`master_fe_addr.hostname:master_fe_http_port/api/get_small_file?...` and calls
`HttpClient::init()` without adding an `https://` scheme or configuring the FE
truststore. With `enable_https=true`, BEs will attempt the existing plain-HTTP
small-file download against the FE HTTPS listener and fail. Until the BE path
is updated to understand HTTPS and trust configuration, this heartbeat field
should continue to carry the FE plain HTTP port, or a separate
scheme/HTTPS-aware field/path should be added and consumed end-to-end.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/meta/MetaService.java:
##########
@@ -163,8 +163,7 @@ public Object put(HttpServletRequest request,
HttpServletResponse response) thro
clientHost, clientPort, machine, portStr);
clientHost = Strings.isNullOrEmpty(clientHost) ? machine : clientHost;
- String url = "http://"; +
NetUtils.getHostPortInAccessibleFormat(clientHost, Integer.valueOf(portStr))
- + "/image?version=" + versionStr;
+ String url = HttpURLUtil.buildInternalFeUrl(clientHost, "/image",
"version=" + versionStr);
Review Comment:
This drops the callback port supplied by the master in `/put?port=...`. The
receiver still validates `portStr`, but now ignores it and uses its own local
`Config.http_port`/`https_port` via `buildInternalFeUrl()`. In the checkpoint
flow the master sends its reachable image port at `Checkpoint.doCheckpoint()`;
if FE HTTP/HTTPS ports differ across nodes during a port override/rolling
change, the follower will try to fetch `/image` from the master using the
follower's local port instead of the master-provided port and image push fails.
Please keep using the validated `portStr` when building this callback URL, with
only the scheme switched for HTTPS if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
