Skip to content

Commit f5fdcda

Browse files
greenc-FNALCopilot
andcommitted
pool: address Copilot review comments for hotfile migration pool selection
- Per #8049 (review) - Log cache-entry lookup failures with stack trace (pass exception object to `LOGGER.warn`, not just `e.getMessage()`) - Handle `null` `ProtocolInfo` in reportFileRequest: fall back to protocol unit "*/*" and empty net unit name instead of throwing NPE - Ensure `netUnitName` is always non-`null`: use client IP address when available via `IpProtocolInfo`, otherwise empty string; guard against `null` socket address/inet address - Enforce non-`null` `netUnitName` in `PoolListByPoolMgrQuery` constructor via `requireNonNull` (consistent with `PoolMgrQueryPoolsMsg` requirement) - Annotate ProtocolInfo parameter as `@Nullable` in FileRequestMonitor interface to document that `null` is a valid input - Update tests: replace `null` `netUnitName` with "" throughout; rename `testRefreshWithNullNetUnitName` to `testRefreshWithUnknownNetUnitName` and assert empty string is passed; remove now-unused `assertNull` import - Update `hotfile-replication.md`: fix `reportFileRequest` snippet to include `protocolInfo` argument; update pool-selection description to reflect protocol/net unit derivation from `ProtocolInfo` instead of `null` Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent b7080b0 commit f5fdcda

5 files changed

Lines changed: 46 additions & 27 deletions

File tree

docs/dev/hotfile-replication.md

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ Any Door (DCap / NFS / WebDAV / xrootd / FTP / HTTP / …)
1818
→ Pool.messageArrived(PoolIoFileMessage)
1919
→ PoolV4.ioFile() ← monitoring happens here
2020
→ queues mover (protocol-specific)
21-
→ FileRequestMonitor.reportFileRequest(pnfsId, currentCount)
21+
→ FileRequestMonitor.reportFileRequest(pnfsId, currentCount, protocolInfo)
2222
```
2323

2424
Because the counting and triggering happen in `PoolV4.ioFile()`, no protocol-specific code
@@ -30,7 +30,8 @@ changes are required to benefit from this feature.
3030

3131
```java
3232
long requestCount = _ioQueue.numberOfRequestsFor(message.getPnfsId());
33-
_fileRequestMonitor.reportFileRequest(message.getPnfsId(), requestCount);
33+
_fileRequestMonitor.reportFileRequest(message.getPnfsId(), requestCount,
34+
message.getProtocolInfo());
3435
```
3536

3637
When `requestCount` reaches or exceeds the configured threshold,
@@ -40,9 +41,12 @@ When `requestCount` reaches or exceeds the configured threshold,
4041
### Pool Selection
4142

4243
The migration job selects target pools by querying PoolManager via `PoolMgrQueryPoolsMsg`,
43-
using `null` for both `protocolUnit` and `netUnitName`. This means selection is based
44-
**solely on the file's storage group and pool-group read preferences** — protocol and
45-
network criteria are intentionally excluded.
44+
deriving `protocolUnit` from the triggering request's `ProtocolInfo` (e.g., `"DCap/3"`) and
45+
`netUnitName` from the client's IP address when available (e.g., `"192.168.1.10"`). When the
46+
client IP is not available (non-IP protocol or unknown), an empty string is used for `netUnitName`,
47+
which causes PoolManager to match any network unit. When `ProtocolInfo` is null (e.g., for
48+
internal pool-to-pool transfers), `protocolUnit` falls back to `"*/*"` and `netUnitName` to `""`
49+
so that selection is based solely on the file's storage group and pool-group read preferences.
4650

4751
`PoolMgrQueryPoolsMsg.getPools()` returns a `List<String>[]` where index 0 is the highest
4852
read-preference level. `PoolListByPoolMgrQuery` selects **only** the first non-empty

modules/dcache/src/main/java/org/dcache/pool/classic/FileRequestMonitor.java

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import diskCacheV111.util.PnfsId;
44
import diskCacheV111.vehicles.ProtocolInfo;
5+
import javax.annotation.Nullable;
56

67
/**
78
* Abstract interface for monitoring file requests in the pool.
@@ -13,8 +14,9 @@ public interface FileRequestMonitor {
1314
*
1415
* @param pnfsId the file identifier
1516
* @param numberOfRequests the number of requests for this file
16-
* @param protocolInfo the protocol info of the request
17+
* @param protocolInfo the protocol info of the request, may be {@code null} if unknown
1718
*/
18-
void reportFileRequest(PnfsId pnfsId, long numberOfRequests, ProtocolInfo protocolInfo);
19+
void reportFileRequest(PnfsId pnfsId, long numberOfRequests,
20+
@Nullable ProtocolInfo protocolInfo);
1921
}
2022

modules/dcache/src/main/java/org/dcache/pool/migration/MigrationModule.java

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1278,20 +1278,34 @@ public synchronized void reportFileRequest(PnfsId pnfsId, long numberOfRequests,
12781278
try {
12791279
cacheEntry = _context.getRepository().getEntry(pnfsId);
12801280
} catch (Exception e) {
1281-
LOGGER.warn("Failed to get cache entry for {}: {}", pnfsId, e.getMessage());
1281+
LOGGER.warn("Failed to get cache entry for {}: {}", pnfsId, e.getMessage(), e);
12821282
return;
12831283
}
12841284
FileAttributes fileAttributes = cacheEntry.getFileAttributes();
12851285

1286-
String protocolUnit = protocolInfo.getProtocol() + "/" + protocolInfo.getMajorVersion();
1287-
if (protocolInfo.getMinorVersion() != 0) {
1288-
protocolUnit += "." + protocolInfo.getMinorVersion();
1289-
}
1290-
1291-
String netUnitName = null;
1292-
if (protocolInfo instanceof IpProtocolInfo) {
1293-
netUnitName = ((IpProtocolInfo) protocolInfo).getSocketAddress().getAddress()
1294-
.getHostAddress();
1286+
String protocolUnit;
1287+
String netUnitName;
1288+
if (protocolInfo != null) {
1289+
protocolUnit = protocolInfo.getProtocol() + "/" + protocolInfo.getMajorVersion();
1290+
if (protocolInfo.getMinorVersion() != 0) {
1291+
protocolUnit += "." + protocolInfo.getMinorVersion();
1292+
}
1293+
if (protocolInfo instanceof IpProtocolInfo) {
1294+
IpProtocolInfo ipProtocolInfo = (IpProtocolInfo) protocolInfo;
1295+
if (ipProtocolInfo.getSocketAddress() != null
1296+
&& ipProtocolInfo.getSocketAddress().getAddress() != null) {
1297+
netUnitName = ipProtocolInfo.getSocketAddress().getAddress()
1298+
.getHostAddress();
1299+
} else {
1300+
netUnitName = "";
1301+
}
1302+
} else {
1303+
netUnitName = "";
1304+
}
1305+
} else {
1306+
// Fall back to PoolSelectionUnit default protocol and net units
1307+
protocolUnit = "*/*";
1308+
netUnitName = "";
12951309
}
12961310

12971311
Collection<Pattern> excluded = new HashSet<>();

modules/dcache/src/main/java/org/dcache/pool/migration/PoolListByPoolMgrQuery.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ class PoolListByPoolMgrQuery
4646
* @param pnfsId the PNFS ID of the file
4747
* @param fileAttributes the file attributes for selection
4848
* @param protocolUnit the protocol unit (e.g., "DCap/3")
49-
* @param netUnitName the network unit name (IP address or null)
49+
* @param netUnitName the network unit name (IP address of the client, or empty string if unknown)
5050
*/
5151
public PoolListByPoolMgrQuery(CellStub poolManager,
5252
PnfsId pnfsId,
@@ -57,7 +57,7 @@ public PoolListByPoolMgrQuery(CellStub poolManager,
5757
_pnfsId = requireNonNull(pnfsId);
5858
_fileAttributes = requireNonNull(fileAttributes);
5959
_protocolUnit = requireNonNull(protocolUnit);
60-
_netUnitName = netUnitName;
60+
_netUnitName = requireNonNull(netUnitName);
6161
}
6262

6363
@Override

modules/dcache/src/test/java/org/dcache/pool/migration/PoolListByPoolMgrQueryTest.java

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import static org.junit.Assert.assertEquals;
44
import static org.junit.Assert.assertFalse;
5-
import static org.junit.Assert.assertNull;
65
import static org.junit.Assert.assertTrue;
76
import static org.mockito.ArgumentMatchers.any;
87
import static org.mockito.Mockito.inOrder;
@@ -166,9 +165,9 @@ public void testRefreshWithMissingStorageInfo() {
166165
}
167166

168167
@Test
169-
public void testRefreshWithNullNetUnitName() throws Exception {
168+
public void testRefreshWithUnknownNetUnitName() throws Exception {
170169
PoolListByPoolMgrQuery poolList = new PoolListByPoolMgrQuery(
171-
poolManager, pnfsId, fileAttributes, "DCap/3", null);
170+
poolManager, pnfsId, fileAttributes, "DCap/3", "");
172171

173172
SettableFuture<PoolMgrQueryPoolsMsg> future = SettableFuture.create();
174173
when(poolManager.send(any(PoolMgrQueryPoolsMsg.class))).thenReturn(future);
@@ -180,7 +179,7 @@ public void testRefreshWithNullNetUnitName() throws Exception {
180179
verify(poolManager).send(queryMsgCaptor.capture());
181180

182181
PoolMgrQueryPoolsMsg queryMsg = queryMsgCaptor.getValue();
183-
assertNull(queryMsg.getNetUnitName());
182+
assertEquals("", queryMsg.getNetUnitName());
184183
}
185184

186185
/**
@@ -196,7 +195,7 @@ public void testSelectsOnlyHighestPreferenceLevel() throws Exception {
196195
// Setup: Two pool groups with different read preferences
197196
// Pool1 is in both groups, pool2-5 only in high-pref group, pool6-10 only in low-pref group
198197
PoolListByPoolMgrQuery poolList = new PoolListByPoolMgrQuery(
199-
poolManager, pnfsId, fileAttributes, "DCap/3", null);
198+
poolManager, pnfsId, fileAttributes, "DCap/3", "");
200199

201200
InOrder inOrder = inOrder(poolManager);
202201

@@ -222,7 +221,7 @@ public void testSelectsOnlyHighestPreferenceLevel() throws Exception {
222221
poolLists[1] = Arrays.asList("pool1", "pool6", "pool7", "pool8", "pool9", "pool10"); // Low pref
223222

224223
PoolMgrQueryPoolsMsg response = new PoolMgrQueryPoolsMsg(
225-
DirectionType.READ, "DCap/3", null, fileAttributes);
224+
DirectionType.READ, "DCap/3", "", fileAttributes);
226225
response.setPoolList(poolLists);
227226
response.setSucceeded();
228227

@@ -275,7 +274,7 @@ public void testSelectsOnlyHighestPreferenceLevel() throws Exception {
275274
@Test
276275
public void testSelectsFirstNonEmptyPreferenceLevel() throws Exception {
277276
PoolListByPoolMgrQuery poolList = new PoolListByPoolMgrQuery(
278-
poolManager, pnfsId, fileAttributes, "DCap/3", null);
277+
poolManager, pnfsId, fileAttributes, "DCap/3", "");
279278

280279
InOrder inOrder = inOrder(poolManager);
281280

@@ -298,7 +297,7 @@ public void testSelectsFirstNonEmptyPreferenceLevel() throws Exception {
298297
poolLists[2] = Arrays.asList("pool1", "pool2"); // Lower preference (should be ignored)
299298

300299
PoolMgrQueryPoolsMsg response = new PoolMgrQueryPoolsMsg(
301-
DirectionType.READ, "DCap/3", null, fileAttributes);
300+
DirectionType.READ, "DCap/3", "", fileAttributes);
302301
response.setPoolList(poolLists);
303302
response.setSucceeded();
304303

0 commit comments

Comments
 (0)