Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions docs/ops/li-bridge-review-comments.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ limitations under the License.

# Review comment dispositions

All 62 original threads have replies with published source decisions. A fresh GraphQL readback across 37 PRs verified every expected reply, found no mismatches and found no new review threads. Resolved status alone was not accepted as proof. Re-fetch after the final publication and check the actual source/test coverage before closing the review.
All 62 original threads have replies with published source decisions. A fresh GraphQL readback across 41 PRs verified every expected reply, found no mismatches and found no new review threads. Resolved status alone was not accepted as proof. Re-fetch after the final publication and check the actual source/test coverage before closing the review.

## Later qualification findings

Expand All @@ -27,7 +27,7 @@ All 62 original threads have replies with published source decisions. A fresh Gr
| [F19: recreated topic already assigned to returning replica](https://github.com/linkedin/kafka/pull/584#issuecomment-5631079569) | Paired identity recovery in 585/586; missing/zero IDs, errors, retries, current/future copies and mixed-batch tests. [Qualification update](https://github.com/linkedin/kafka/pull/586#issuecomment-5638005151). | All four revision-4 record checks passed, but full final-source and wrapper qualification remain open. |
| [F20: rotated protocol logs omitted](https://github.com/linkedin/kafka/pull/584#issuecomment-5638004839) | PR 584 retains and scans hourly rotations; 586 is restacked on it. New positive and negative tests fail before the fix and pass afterward. | The previous failed CI job stays failed. Re-run the corrected collector. |
| [F21: churn exits during controller movement](https://github.com/linkedin/kafka/pull/586#issuecomment-5638182322) | PR 584 fixes the workload retry policy without changing the upstream broker response. [Test and code update](https://github.com/linkedin/kafka/pull/584#issuecomment-5638482746). | The complete revision-4 process run and audit pass; final F22/wrapper qualification remains required. |
| F22: bridge-state MBeans register by default | Paired [588](https://github.com/linkedin/kafka/pull/588)/[589](https://github.com/linkedin/kafka/pull/589) add a default-off diagnostics flag. Tests cover disabled registration, enabled readings, restart scope, KRaft and cleanup. | All 72 Python tests pass. The wrapper mapping still needs matching-jar qualification. |
| F22: bridge-state MBeans register by default | Paired [588](https://github.com/linkedin/kafka/pull/588)/[589](https://github.com/linkedin/kafka/pull/589) add a default-off diagnostics flag. Tests cover disabled registration, enabled readings, restart scope, KRaft and cleanup. | All 72 Python tests pass. The matching-jar wrapper suite now passes 133 tests; the complete final-source verifier remains open. |

## PR 541

Expand Down Expand Up @@ -101,7 +101,7 @@ All 62 original threads have replies with published source decisions. A fresh Gr
| Comment | Decision | Code/test evidence | Reply |
|---|---|---|---|
| [1](https://github.com/linkedin/kafka/pull/551#discussion_r3925877328) | Capture the post-truncation offset and size while holding the log lock. | UnifiedLog.truncateTo; truncation test and mixed stale-leader process scenario. | [reply](https://github.com/linkedin/kafka/pull/551#discussion_r3984229174) |
| [2](https://github.com/linkedin/kafka/pull/551#discussion_r3925877401) | Convert malformed numbers to ConfigException. Keep empty lists as an intentional way to disable a bucket dimension. | KafkaConfigTest.testInvalidRequestMetricBuckets and testEmptyRequestMetricBuckets; empty is not rejected because it is supported. | [reply](https://github.com/linkedin/kafka/pull/551#discussion_r3984229326) |
| [2](https://github.com/linkedin/kafka/pull/551#discussion_r3925877401) | Reject empty bucket lists with ConfigException, as requested. The previous reply and ledger incorrectly claimed support for empty lists. | KafkaConfigTest.testEmptyRequestMetricBuckets asserts rejection; malformed, negative and unordered cases have separate assertions. | [correction](https://github.com/linkedin/kafka/pull/551#discussion_r3992669600) |
| [3](https://github.com/linkedin/kafka/pull/551#discussion_r3925877442) | Close cumulative counters when their associated ingress metric is removed. | BrokerTopicMetricsTest.testCloseMetricClosesCumulativeIngressCounters; moved to the storage-metrics split. | [reply](https://github.com/linkedin/kafka/pull/551#discussion_r3984229507) |
| [4](https://github.com/linkedin/kafka/pull/551#discussion_r3926413418) | Reject negative and non-increasing boundaries. | KafkaConfigTest.testRequestMetricBucketsMustBeOrderedAndNonNegative. | [reply](https://github.com/linkedin/kafka/pull/551#discussion_r3984229700) |
| [5](https://github.com/linkedin/kafka/pull/551#discussion_r3926413465) | Assert on the specific metric names, not global registry size. | BrokerTopicMetricsTest uses per-metric presence checks; storage split tests pass. | [reply](https://github.com/linkedin/kafka/pull/551#discussion_r3984229872) |
Expand Down
Loading
Loading