github-actions[bot] commented on code in PR #66710:
URL: https://github.com/apache/doris/pull/66710#discussion_r3772672584
##########
fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergScanRange.java:
##########
@@ -153,6 +153,13 @@ public Optional<String> getPath() {
return Optional.ofNullable(path);
}
+ @Override
+ public Optional<String> getFileAffinityKey() {
+ return serializedSplit == null
Review Comment:
The native `$position_deletes` path never reaches this affinity arm.
`buildPositionDeleteRange()` can byte-split one plain Parquet/ORC delete file,
but it stores the reader type only in `positionDeleteFileFormat`; this class's
separate `fileFormat` string stays empty. Consequently every slice returns
`Optional.empty()` here and can still land on different BEs, even though
`IcebergPositionDeleteSysTableReader` uses the same Parquet/ORC file-metadata
cache this change is trying to reuse. Please derive the key from the native
position-delete format for `CONTENT_POSITION_DELETE`, while explicitly keeping
Puffin/JNI ranges excluded, and cover both a multi-slice delete file and the
Puffin case.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/scan/FederationBackendPolicy.java:
##########
@@ -310,6 +317,24 @@ public Multimap<Backend, Split>
computeScanRangeAssignment(List<Split> splits) t
return assignment;
}
+ private void assignSplit(ListMultimap<Backend, Split> assignment, Backend
backend, Split split) {
+ assignment.put(backend, split);
+ assignedWeightPerBackend.put(backend,
+ assignedWeightPerBackend.get(backend) +
split.getSplitWeight().getRawValue());
+ }
+
+ private Backend getFileAffinityBackend(Split split) {
+ if (!isFileAffinityEligible(split)) {
+ return null;
+ }
+ return consistentHash.getNode(split, 1).get(0);
Review Comment:
`split_assigner_virtual_node_number` is a mutable unrestricted integer. If
it is set to `0` (or negative), `ConsistentHash.addNode()` creates an empty
ring and `getNode(..., 1)` returns an empty list, so this new unconditional
`.get(0)` makes otherwise valid round-robin Parquet/ORC scans fail with
`IndexOutOfBoundsException`. The cache key also omits the vnode count, so an
empty ring can survive restoring a positive value while an old positive ring
can mask an invalid update. Please enforce a positive value at the config
boundary and make cache reuse reflect vnode-count changes (or invalidate it),
with cold-zero and recovery coverage.
##########
fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/scan/ConnectorScanRange.java:
##########
@@ -46,6 +46,16 @@ default Optional<String> getPath() {
return Optional.empty();
}
+ /**
+ * Returns a stable physical-file key when ranges of this connector use a
native Parquet/ORC reader and can
+ * reuse file-level metadata on one BE. This key only influences FE
placement; it is not a versioned identity
+ * for a BE metadata cache. Virtual and JNI ranges must keep the empty
default even if {@link #getPath()}
+ * returns a routing identifier.
+ */
+ default Optional<String> getFileAffinityKey() {
Review Comment:
This adds a public connector-SPI method, but both the base and this PR still
advertise connector API `5.0`. The local POM and `ApiVersionGate` contract
define *any* SPI surface change, including additions, as a major change;
otherwise a plugin compiled against this method can pass an older 5.x FE's gate
and fail when it invokes a method that FE's parent-first interface does not
have. `ConnectorPluginSurfaceTest` also omits `ConnectorScanRange`, so the
current baseline cannot flag this drift. Please bump the connector API major
and include this reachable scan surface in the recorded baseline/version
assertion.
##########
fe/fe-core/src/test/java/org/apache/doris/datasource/split/SplitAssignmentTest.java:
##########
@@ -76,7 +76,8 @@ void setUp() {
mockSplitToScanRange,
locationProperties,
pathPartitionKeys,
- true
+ true,
+ false
Review Comment:
Both `SplitAssignmentTest` instances pass the new flag as `false`, so this
suite never exercises the lazy production path that enables affinity before
assignment; the policy tests enable their `FileSplit`s manually and bypass this
wiring. The added locality test is also vacuous because it leaves affinity
disabled, so it would pass even if the host fence were removed. Please add a
positive real-`FileSplit` lazy case, enable affinity before the locality
assertion, and cover the eager call at an equivalent production boundary.
--
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]
