Add Kafka publication partitioner analyzer and code fixes - #4255
Add Kafka publication partitioner analyzer and code fixes#4255lillo42 wants to merge 12 commits into
Conversation
Code Review — Add Kafka Analyzer (BRT006–BRT008)Thanks for this — the three Kafka partitioner rules plus code fixes are a genuinely useful addition, the docs (BRT006–008) are excellent, and the test coverage for the happy-path and single-diagnostic cases is solid. A few things worth addressing before merge, one of which I believe is a real correctness bug. 🔴 Correctness — visitor recurses into every object creation, causing false positives at the wrong location
public override void VisitObjectCreation(IObjectCreationOperation operation)
{
if (operation.Type!.Accept(new ChildOfVisitor(KafkaPublicationClassName, ...)))
{
PublicationName = operation.Type.Name;
IsKafkaPublication = true;
}
// base walks the children ... ALWAYS, even when this operation is not a KafkaPublication
base.VisitObjectCreation(operation);
}Compare with the existing sibling Concrete failure — a very common Brighter registration shape: var factory = new KafkaProducerRegistryFactory(
configuration,
new[] { new KafkaPublication() }); // partitioner missing
The same happens with Fix: move 🟠 Test coverage gapAll analyzer tests declare the publication at statement level ( 🟡 Minor points
Nice work overall — the correctness fix is small and the rest are polish. 👍 🤖 Automated review. I verified the visitor-recursion difference against the existing |
lillo42
changed the title
Review: Kafka publication partitioner analyzer (BRT006–BRT008)Nice addition — the rules are well-motivated, the docs ( A few things I'd like to see addressed before merge, plus some smaller notes. Correctness / potential bugs1. if (operation.Target is IPropertyReferenceOperation propertyReference &&
propertyReference.Property.Name == BrighterAnalyzerGlobals.PartitionerProperty)Because Two options:
2. False positive when var publication = new KafkaPublication { Topic = new RoutingKey("x") };
publication.Partitioner = Partitioner.Murmur2Random; // BRT006 still firesOnly the object-creation operation is analysed, so this common pattern (and configuring a publication inside a helper method) reports a warning the user has already fixed. Worth either handling the enclosing block / variable-initializer case, or at minimum documenting the limitation in 3. The BRT007/8 code fix can emit code that doesn't compile — ExpressionSyntax newValue = assignment.Right switch
{
MemberAccessExpressionSyntax memberAccess => memberAccess.WithName(newName),
_ => newName
};
This is exactly the class of bug that 4. Nullability — Consumer impactBRT006 fires on every
Relatedly: the BRT006 code fix changes runtime behaviour. The property default is None of the descriptors set Packaging
Minor: Also: new rules conventionally land in Test coverageHappy paths are covered well and the nested-object-creation cases are a good inclusion. Gaps I'd want filled, mostly because they're the paths where the bugs above live:
Nits
Overall the rules, docs, and structure are solid. Items 1 and 3 are the ones I'd treat as blocking, and I'd settle the BRT006 severity / "non-breaking" question before this ships. 🤖 Review by Claude Code |
- Only treat Partitioner assignments on KafkaPublication itself as the publication's partitioner (a nested object's own Partitioner property, e.g. Confluent's ProducerConfig, no longer suppresses BRT006) - Don't report BRT006 when the partitioner is assigned to the same local later in the same block - Don't offer the BRT007/8 code fix when the assigned expression is neither a member access nor a bare identifier (would not compile) - Verify fixed code compiles in code-fix tests (CompilerDiagnostics.Errors) - Add tests: Partitioner.Random negative, nested config with its own Partitioner, set-after-construction, code fix without the Kafka using - Document in BRT006.md that the code fix changes partition assignment and note the helper-method limitation - Add helpLinkUri for BRT006-BRT008, render generic publications as KafkaPublication<T>, drop no-op packaging metadata from CodeFixes.csproj
Review: Kafka publication partitioner analyzer and code fixes (1/2)Overall this is a well-put-together contribution. The analyzer follows the existing Comments below, roughly in priority order. 1. BRT007/BRT008 silently miss the post-construction assignment they warn about elsewhere
if (!visitor.IsPartitionerAssigned)
{
if (IsPartitionerAssignedAfterConstruction(operation)) return; // <-- bails out entirely
report BRT006;
}
else if (visitor.IsConsistentRandom) { ... }
var publication = new KafkaPublication();
publication.Partitioner = Partitioner.Consistent; // no diagnostic at allproduces no warning, while the equivalent initializer form produces BRT008. That's an inconsistency users will notice, and it's an easy accidental suppression: assign the discouraged value on the next line and the rule goes quiet. Worse, it's exactly the shape a user lands on after "fixing" BRT006 by setting the partitioner explicitly. Suggestion: have the helper return the matching A test for the discouraged-value-after-construction case would pin this down. 2. The analyzer allocates a visitor for every
|
Review: Kafka publication partitioner analyzer and code fixes (2/2)5. New rules were added to
|
Review: Kafka publication partitioner analyzer (BRT006-BRT008)Nice piece of work overall. Worth calling out what is genuinely good before the feedback:
Findings below, roughly in severity order. (Reviewed statically - this run could not execute 1.
|
|
(review continued from the previous comment) 4. Release tracking: new rules went into
|
Review: Kafka publication partitioner analyzer (BRT006–BRT008)Solid work. Several things here are better than the existing analyzers and worth keeping as the house pattern: the I have not relitigated the two decisions recorded in the description (BRT006 at 1. BRT006 false-positives on subclasses that set
|
| value | behaviour |
|---|---|
consistent |
CRC32 of key; empty/NULL keys → a single partition |
consistent_random |
CRC32 of key; empty/NULL keys randomly partitioned |
murmur2 |
Java-compatible Murmur2; NULL keys → a single partition |
murmur2_random |
Java-compatible Murmur2; NULL keys randomly partitioned — "functionally equivalent to the default partitioner in the Java Producer" |
- The Java-compatibility claim is on the wrong value.
murmur2_random, notmurmur2, is what the standard Java producer does. BRT008.md asserts the opposite. Murmur2reintroduces the exact hot-partition failure mode the doc warns about, funnelling every keyless message onto one partition. If hot partitions are the driver, BRT008 should recommendMurmur2Randomtoo (matching BRT006/BRT007), withMurmur2offered only as the strict like-for-like swap for callers who deliberately want NULL-keyed messages pinned.
Separately, the premise shared by BRT007 and BRT008 — that CRC32 "spreads keys less evenly" than Murmur2 — isn't something librdkafka or the Kafka docs claim. The documented differentiator between the consistent* and murmur2* families is cross-client key compatibility with the Java producer, not hash quality. These strings ship in the IDE and justify a change that re-partitions a live topic, so I'd lead with the interop argument and soften or drop the distribution-quality one.
4. Test coverage gaps, all on the code-fix side
- No Fix All test. Both providers override
GetFixAllProvider(), the description discusses Fix All explicitly, andAddInitializerExpressiondoes manual trivia surgery — yet no test puts twoKafkaPublications in one document.CSharpCodeFixTestsupports this viaNumberOfFixAllIterations/BatchFixedCode. - No single-line-initializer test.
AddInitializerExpressionderivesendOfLineTriviawithSkipWhile(t => !t.IsKind(EndOfLineTrivia)); fornew KafkaPublication { Topic = x }that's empty andcommentTriviabecomes trailing whitespace, which then gets moved onto the separator. The formatter probably rescues it, but only multi-line initializers are exercised. - No "no fix offered" test for the
CanRewritebail-out (e.g.Partitioner = (Partitioner)Partitioner.Consistent) — pin it withFixedCode = TestCode. - No tests for the false positives in §1 and §2.
5. Performance
RegisterCompilationStartAction already resolves the KafkaPublication symbol and discards it. IsKafkaPublicationType then walks the base chain doing two string comparisons per level for every ObjectCreation and every Partitioner-named SimpleAssignment in the compilation. Capture the INamedTypeSymbol in the compilation-start closure and use SymbolEqualityComparer.Default instead — same for the Partitioner enum symbol that §2 needs anyway. It also drops the assembly-name string dependency, so the check survives a rename.
6. Smaller things
- The
IMemberInitializerOperationcomment is wrong (KafkaPublicationPartitionerAnalyzer.cs:148-149): it isn't aboutwithexpressions, it represents a nested member initializer —new Holder { Publication = { Partitioner = ... } }. The guard is harmless, but that form is a silent false negative: neitherAnalyzerObjectCreation(nonew KafkaPublication) norAnalyzeAssignment(skipped by the guard) fires. Handle it or list it with the other documented limitations. HasPartitionerAssignmentAfterConstructioncompares symbols, not instances for field/property targets, soa.Pub = new KafkaPublication(); b.Pub.Partitioner = ...;suppresses BRT006 ona. Minor; a comment would do.AnalyzerReleases: BRT006–BRT008 were added toShipped.mdunder the existing## Release 1.0heading whileUnshipped.mdsits empty. Convention is that new rules land in Unshipped and the release process promotes them; editing a shipped section retroactively rewrites what 1.0 contained.- Licence headers:
KafkaPublicationPartitionerVisitor.cscredits Aboubakr Nasef, the analyzer and both code fixes credit Ian Cooper, on a PR authored by @lillo42 — looks like template copy-paste. - XML docs are absent on the new public types/members. Consistent with the existing analyzer files, so not a regression, but
.agent_instructions/documentation.mdasks for docs on exports and the checklist item is unticked — flagging so it's a conscious skip. MissingPartitionerCodeFixProvider.cs:67:var target = ...Murmur2RandomPartitionerValue;is a local that's never anything else — inline it, or drop the parameter fromAddPartitionerAsync.- New files use file-scoped namespaces while the surrounding analyzer files use block-scoped. Not wrong, just inconsistent within the folder.
Summary. §1 and §2 are what I'd want addressed before merge — both can make a code fix silently write incorrect code, the worst failure mode for an analyzer shipping Fix All. §3 is a defect in the guidance rather than the code, but that guidance is what justifies re-partitioning a production topic. §4–§6 are follow-ups.
Reviewed statically — dotnet build/dotnet test weren't runnable in this environment, so I have not executed the suite.
Review: Kafka publication partitioner analyzer (BRT006–BRT008)Overall this is a well-built analyzer. The things that usually go wrong in Roslyn analyzers are handled correctly here: symbols are resolved once in I couldn't build in this environment, so the notes below are from static reading. I'm treating the two "Maintainer decisions" in the PR description as settled and not relitigating them. Bugs1. var lastExpression = initializer.Expressions.Last();
if (initializer.Expressions.Count == 0)
{
return initializer.WithExpressions(
SyntaxFactory.SingletonSeparatedList(expression));
}2. A trailing comma sends a multi-line initializer down the single-line branch — The branch is chosen by whether the last expression's trailing trivia contains an new KafkaPublication
{
Topic = new RoutingKey("x"),
NumPartitions = 3,
}the newline hangs off the trailing
3. It's not mentioned in the PR description and has nothing to do with the analyzer. It's also broken as written: Correctness / accuracy4. The sample comment is wrong about the client default — repeated in all 10 sample edits
librdkafka (and therefore Confluent.Kafka) defaults Separately: the same two-line comment is copy-pasted into 10 sample files. One explanation in one canonical sample ( 5. The BRT007 rule The descriptor says:
but BRT007.md correctly identifies the real rationale as cross-client hash compatibility (both variants already spread keyless messages randomly; the hot-partition argument is BRT008's). The descriptor text is what shows in the IDE tooltip, so it's the version most users will actually read. Worth aligning it with the doc. 6. Five sample files lost their UTF-8 BOM — The diff shows the leading BOM stripped from the first line of each. Harmless but unrelated churn that shows up in the diff; probably an editor setting. Release tracking7. New rules were added to The Roslyn release-tracking convention (RS2000/RS2001) is that new rules go into Performance / analyzer hygiene8.
9. BRT006 squiggles the entire object creation —
False negatives worth documenting10. A subclass from a referenced assembly always trips BRT006 with no fixable remedy
TestsCoverage is solid — ~30 analyzer cases plus 15 code-fix cases, with the tricky ones (target-typed
Minor: One suggestion on the maintainer decisionsNot reopening either decision — but since both fixes change runtime behaviour, putting that consequence in the code-action title costs nothing and is the one place the IDE guarantees the user sees it before clicking (the rule title: $"Set 'Partitioner' to 'Murmur2Random' (re-partitions the topic)"Nice work overall — the analyzer logic and the docs are both above the bar. Finding 1 is the only one I'd call blocking. |
Review: Kafka publication partitioner analyzer + code fixesOverall this is high-quality analyzer work. The things I'd normally flag are already handled: symbol resolution is hoisted into A few things I think should be addressed before merge, then some smaller notes. 1. The packaged .editorconfig files were not updated (blocking, I think)
Both files still list only BRT001–BRT005. So:
Please add the three new IDs to both files. (Unrelated and pre-existing, so out of scope here, but worth a follow-up issue: neither file contains 2. New rules should go in AnalyzerReleases.Unshipped.mdBRT006–008 were appended to 3. ConstructorAssignsPartitioner over-scans for primary-constructor subclasses
class OrdersPublication(string topic) : KafkaPublication
{
public void Reconfigure() => Partitioner = Partitioner.Consistent; // not a ctor assignment
}
var p = new OrdersPublication("orders"); // BRT006 suppressedAny 4. Nested-block assignment produces two contradictory diagnostics
var pub = new KafkaPublication(); // BRT006 (false positive)
if (legacy) { pub.Partitioner = Partitioner.Consistent; } // BRT008Applying both fixes yields 5. Test coverage: the shape every sample uses isn't testedEvery publication in [
new KafkaPublication<GreetingEvent> { Topic = …, NumPartitions = 3, … }
]That's the canonical Brighter shape and the exact path Smaller notes
On the two maintainer decisionsBoth are reasonable calls and I won't relitigate them. One observation ties back to #1 though: the argument for shipping BRT006 at Warning-by-default rests on each diagnostic being individually suppressible and on consumers being able to opt out — which is precisely what the un-updated I wasn't able to build or run the test suite in this environment, so the notes above come from reading the code rather than from a red/green run. |
|
Review: Kafka publication partitioner analyzer (BRT006–BRT008) Reviewed by reading the diff and the surrounding code; I was not able to build or run the test suite in this environment, so everything below comes from static reading. Overall this is high-quality analyzer work. A few things stand out as done properly rather than approximately:
The two "maintainer decisions" in the description are yours to make and I am not re-litigating them. Item 2 below is about the docs for that decision, not the decision. Findings 1. The check is class OrdersPublication : KafkaPublication
{
public OrdersPublication()
{
// configures a *different* publication; OrdersPublication.Partitioner is never set
_audit = new KafkaPublication { Partitioner = Partitioner.Murmur2 };
}
}
var p = new OrdersPublication(); // BRT006 suppressed — but nothing set its PartitionerThe comment above the method covers the "local named 2. The BRT006 remediation is not reachable warning-free, and the doc does not quite say so The stated purpose of BRT006 is to make the choice visible. But the only way to make the current behaviour visible is That is a coherent position, but 3. New rules were added to
4. The BRT007 descriptor text argues something different from BRT007.md The descriptor ( 5. A conditional assignment fully silences BRT006 —
var p = new KafkaPublication();
if (useKeys) p.Partitioner = Partitioner.Murmur2; // BRT006 silenced on every pathThe doc advertises nested-block support as a feature, which it is — but the "only on one branch" and "textually later but executed earlier in a loop" cases both read as satisfied. This looks like a deliberate false-negative trade-off (better than false positives here); worth one sentence in 6. The fix produces two different layouts for near-identical input
7.
8. Coverage gap: ambiguous
Nits
|
|
Descriptor wording vs BRT007.md (finding 4): this one is a deliberate maintainer decision, not an oversight. The rule's stated position is that the Two-layout fix output (finding 6, second half): the empty argument list is now dropped ( |
Review: Kafka publication partitioner analyzer (BRT006–BRT008)I read the analyzer, the visitor, both code-fix providers, the packaging changes, the docs and all the tests. This is high-quality analyzer work — a few things worth calling out as genuinely well done before the findings:
Not re-litigating the two maintainer decisions in the PR body (BRT006 at 1. The IDE-facing rule descriptions contradict BRT008.md on hot partitions
But
So for the value the rule recommends, the headline benefit in the description is not delivered. Relatedly, the strongest justification — that murmur2 is what the Java producer / librdkafka default uses, so a key lands on the same partition across clients — is the lead argument in 2. "spreads keys more evenly than CRC32" is an unsupported claimThis appears in both rule descriptions, in 3.
|
1 participant
Description
Adds a Kafka publication partitioner analyzer with three new diagnostics:
KafkaPublicationis created without an explicitPartitionerassignment.Partitioner.ConsistentRandomis used and recommendsPartitioner.Murmur2Randomfor new publications.Partitioner.Consistentis used and recommendsPartitioner.Murmur2for new publications.This also introduces a new
Paramore.Brighter.Analyzer.CodeFixesproject and ships it in the analyzer NuGet package. The code fixes can add the recommendedPartitioner.Murmur2Randomassignment or replace legacyConsistentRandom/Consistentvalues with the recommended Murmur2 alternatives.Documentation for BRT006–BRT008 is included, together with analyzer and code-fix tests.
Related Issues
PartitionerRequired onKafkaPublicationin v11 and Default Guidance to Murmur2 #4218Type of Change
Checklist
Additional Notes
Existing publications that intentionally rely on
ConsistentRandomorConsistentcan keep their current partition assignment and suppress the warning if preserving the current key-to-partition mapping is required.Maintainer decisions
Two points raised in review are deliberate decisions, not oversights:
Warning, enabled by default. EveryKafkaPublicationcreated without an explicitPartitioneris flagged, so consumers building withTreatWarningsAsErrors(this repo included) will see build failures on upgrade until they either set the partitioner explicitly or suppress the rule. This is intentional: omitting the assignment silently selects theConsistentRandomdefault — the same value BRT007 discourages — so it is reported at the same severity (the argument is written up inBRT006.md).