devmadhuu commented on PR #10696: URL: https://github.com/apache/ozone/pull/10696#issuecomment-5011339570
> It would be nice to respect configration property ozone.om.db.checkpoint.use.inode.based.transfer and switch to the old endpoint if it's false. Thanks @jojochuang for catching that. Fixed it in latest push. > Sample repro test for the Bugbot finding about **leader change mid chunked transfer** (`ReconRDBSnapshotProvider` re-resolves `leaderInfoSupplier.get()` on every chunk while `checkLeaderConsistency` runs only once). > > Pushed to my fork: [jojochuang@fa988a0](https://github.com/jojochuang/ozone/commit/fa988a0df78fe19fd208684efc9c37aeec80beb1) > > The test simulates a two-chunk transfer where the supplier returns `om-leader-a` for chunk 1 and `om-leader-b` for chunk 2. It asserts the **safe** behavior (pin original leader for all chunks **or** reset the candidate dir). On current code it **fails**, which demonstrates the bug: > > ``` > leadersUsedPerChunk=[om-leader-a, om-leader-b], initCountBefore=1, initCountAfter=1 > ``` > > Run locally: > > ```shell > mvn -pl :ozone-recon test \ > -Dtest=TestReconRDBSnapshotProvider#testLeaderChangeMidTransferUsesNewLeaderWithoutResettingCandidate \ > -DskipShade -DskipRecon -DskipDocs > ``` > > Core test code (`TestReconRDBSnapshotProvider.java`): > > ```java > @Test > public void testLeaderChangeMidTransferUsesNewLeaderWithoutResettingCandidate( > @TempDir File snapshotDir) throws IOException { > ServiceInfo leaderA = mock(ServiceInfo.class); > when(leaderA.getHostname()).thenReturn("om-leader-a"); > when(leaderA.getPort(any())).thenReturn(9874); > ServiceInfo leaderB = mock(ServiceInfo.class); > when(leaderB.getHostname()).thenReturn("om-leader-b"); > when(leaderB.getPort(any())).thenReturn(9874); > > AtomicInteger supplierCalls = new AtomicInteger(); > Supplier<ServiceInfo> leaderSupplier = () -> > supplierCalls.getAndIncrement() == 0 ? leaderA : leaderB; > > List<String> leadersUsedPerChunk = new ArrayList<>(); > > ReconRDBSnapshotProvider provider = > new ReconRDBSnapshotProvider(snapshotDir, null, false, > HttpConfig.Policy.HTTP_ONLY, false, true, leaderSupplier) { > @Override > public void downloadSnapshot(String leaderNodeID, File targetFile) > throws IOException { > ServiceInfo leader = leaderSupplier.get(); > leadersUsedPerChunk.add(leader.getHostname()); > if (leadersUsedPerChunk.size() == 1) { > Map<String, String> chunkOne = new HashMap<>(); > chunkOne.put("fromLeaderA.sst", "partial-a"); > chunkOne.put("CURRENT", "MANIFEST"); > writeTar(targetFile, chunkOne); > } else { > Map<String, String> chunkTwo = new HashMap<>(); > chunkTwo.put(HddsServerUtil.OZONE_RATIS_SNAPSHOT_COMPLETE_FLAG_NAME, ""); > writeTar(targetFile, chunkTwo); > } > } > }; > > long initCountBefore = provider.getInitCount(); > provider.downloadDBSnapshotFromLeader("leader-a-node-id"); > > boolean pinnedLeader = leadersUsedPerChunk.stream() > .allMatch("om-leader-a"::equals); > boolean resetCandidate = provider.getInitCount() > initCountBefore; > assertTrue(pinnedLeader || resetCandidate, > "When the resolved OM leader changes mid-transfer, Recon must either " > + "pin the original leader for all chunks or reset the candidate " > + "dir before continuing; leadersUsedPerChunk=" + leadersUsedPerChunk > + ", initCountBefore=" + initCountBefore > + ", initCountAfter=" + provider.getInitCount()); > } > ``` > > This is a repro-only sketch for discussion — not intended to land as-is on the PR branch until the fix is in place. Thanks @jojochuang for the repro and putting details here.. Agree the concern is valid at the framework level, though it isn't reachable for Recon today: Recon requests `includeSnapshotData=false`, so the server sets `maxTotalSstSize=Long.MAX_VALUE` and sends the active OM DB as a single batch with the completion flag in the same response. That makes the transfer always single-chunk — `downloadSnapshot` runs once and the leader resolves once, so there's no second chunk that could come from a different leader (which is why your repro has to stub two chunks). Most important is this behavior of Recon for downloading active OM DB is same as OM follower for active OM DB where OM follower also downloads the active OM DB in one single tar ball transfer. Only snapshots data and checkpoint logs are downloaded by OM follower in multi chunked transfer fashion. That said, relying on single-chunk is fragile. `ReconRDBSnapshotProvider` re-resolves the leader via the supplier on every call instead of pinning the `leaderNodeID` the loop was invoked with (like `OmRatisSnapshotProvider` does). I'll pin the leader for the whole transfer so it's correct by construction regardless of chunk count. Will push in the next revision. -- 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]
