feat(logging): bridge native IVF-PQ diagnostics to SLF4J - #75
Conversation
83f03b6 to
00b5f42
Compare
| timing.prepare = elapsed_since(prepare_started); | ||
|
|
||
| let mut heaps = (0..nq).map(|_| TopKHeap::new(k)).collect::<Vec<_>>(); | ||
| if timing_enabled { |
There was a problem hiding this comment.
[major] End read measurement and emit diagnostics on error paths
After begin_read_metrics(), several fallible operations can return early through ?, including list_payload_len, streamed reads, batch_read_end, and read_inverted_list_payloads. end_read_metrics() is only called on the successful path.
Consequently, failed searches emit no timing diagnostic and leave metrics enabled on the reader, so later operations continue collecting measurements and paying instrumentation overhead.
Please use an RAII guard or an equivalent finally-style scope to end measurement on every exit path. When timing is enabled, it would also be useful to emit a record with status=error, the completed phase timings, I/O metrics, and a non-sensitive error category.
| mut observe_ephemeral_precomputed_lists: impl FnMut(usize), | ||
| #[cfg(test)] distance_table_builds: Option<&std::sync::atomic::AtomicUsize>, | ||
| ) -> io::Result<(Vec<i64>, Vec<f32>)> { | ||
| let timing_enabled = std::env::var_os("PAIMON_VINDEX_LOG_IVFPQ_TIMING").is_some(); |
There was a problem hiding this comment.
[major] Clarify or cover the single-query IVF-PQ search paths
PAIMON_VINDEX_LOG_IVFPQ_TIMING is only checked in the batch implementation. The JNI search and searchWithRoaringFilter methods use the single-query reader path, so those operations never produce IVF-PQ timing diagnostics.
Please either instrument search_with_reader_filter as well, or make the batch-only scope explicit by renaming the flag and documentation to something such as PAIMON_VINDEX_LOG_IVFPQ_BATCH_TIMING.
| throw new AssertionError( | ||
| "timing record leaked to stdout instead of the log bridge:\n" + out); | ||
| } | ||
| if (!err.contains(TIMING_MARKER)) { |
There was a problem hiding this comment.
[major] Assert SLF4J delivery rather than only checking System.err
This assertion only proves that the marker reaches Java's System.err. Both slf4j-simple and the fallback in NativeLogBridge.log() write to that stream, so an exception in the SLF4J call can fall back to System.err.println(message) and still pass this test.
Please use a recording SLF4J binding/appender and assert the logger name, level, and message directly. The stderr fallback should be exercised separately by an explicit failure-path test.
| emit_log(LogLevel::Info, String::from_utf8_lossy(&buf).trim_end()); | ||
| } | ||
|
|
||
| if !by_residual && std::env::var_os("PAIMON_VINDEX_LOG_IVFPQ_BATCH_REUSE").is_some() { |
There was a problem hiding this comment.
[minor] Cover the table-reuse emitter in the JNI bridge test
This is the second diagnostic emitter migrated to emit_log, but the smoke test only enables PAIMON_VINDEX_LOG_IVFPQ_TIMING and builds an L2 residual index. It therefore never enters this !by_residual branch.
Please add a forked-JVM case using an inner-product or cosine index with PAIMON_VINDEX_LOG_IVFPQ_BATCH_REUSE=1, and verify that the ivfpq_batch_table_reuse record is delivered through the Java logging bridge.
There was a problem hiding this comment.
Thanks. This extra forked-JVM case is not needed: the JNI bridge is a process-wide, message-agnostic sink, and the existing timing smoke test already exercises that transport end to end. The table-reuse behavior itself is covered by the core batch-reuse tests. A second IP/cosine JVM search would repeat the same bridge path without covering different delivery behavior.
|
+1 |
Summary
Route opt-in native IVF-PQ diagnostics through Java SLF4J/log4j when the JNI library is loaded, while retaining a stderr fallback for non-JNI consumers.
Changes
PAIMON_VINDEX_LOG_IVFPQ_BATCH_TIMING.JNI_OnLoadand map native levels to SLF4J.Testing
cargo fmt --all -- --checkmvn -f java/pom.xml test-compilecargo test -p paimon-vindex-core(GitHub x86 CI; local ARM is blocked by an existing unstable NEON intrinsic onmain)Notes