Repository navigation
Conversation
utafrali
left a comment
utafrali
left a comment
There was a problem hiding this comment.
The one-line production fix is correct and clearly improves diagnostics by not misattributing constructor failures to classpath issues, and the added tests lock in the intended behavior including the regression case. Only minor test-file hygiene suggestions (file mode, exception-assertion style, comment wording).
| @@ -0,0 +1,185 @@ | |||
| /* | |||
There was a problem hiding this comment.
This new file was committed with mode 100755 (executable). Java source files in this repo are 100644 — please chmod 644 and re-commit so the file mode matches the rest of store/src/test/java.
There was a problem hiding this comment.
Fixed in 143ae96 — the file is now committed with mode 100644, matching the rest of store/src/test/java. Good catch, thank you!
| return new MessageStorePluginContext(new MessageStoreConfig(), null, null, brokerConfig, null); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
The try { build(...); fail(...); } catch (RuntimeException e) { assertThat(...) } pattern can be replaced by AssertJ's assertThatThrownBy for a cleaner and less error-prone form, e.g.:
assertThatThrownBy(() -> MessageStoreFactory.build(context, null))
.isInstanceOf(RuntimeException.class)
.hasMessageContaining("Failed to initialize message store plugin")
.hasMessageContaining("NoSuchPluginClassSurelyMissing")
.hasCauseInstanceOf(ClassNotFoundException.class);This also removes the risk of a silently passing test if fail(...) ever gets refactored out. Same applies to testPluginConstructorThrows.
There was a problem hiding this comment.
Done in 143ae96 — both testPluginClassNotExist and testPluginConstructorThrows now use assertThatThrownBy chains. For the throwing-constructor case the chain also carries the former root-cause assertions via hasCauseInstanceOf(InvocationTargetException.class) + hasRootCauseInstanceOf(IllegalStateException.class) + hasRootCauseMessage("boom"). All 6 tests pass locally (mvn -pl store -am -Dtest=MessageStoreFactoryTest test).
| assertThat(outer.context).isSameAs(context); | ||
| assertThat(inner.context).isSameAs(context); | ||
| } | ||
|
|
There was a problem hiding this comment.
The Javadoc comment "AbstractPluginMessageStore leaves a few MessageStore methods unimplemented in this branch" is ambiguous — "this branch" reads like the PR branch, but the intent is presumably "in the current develop state of AbstractPluginMessageStore". Consider rephrasing to something like "AbstractPluginMessageStore does not delegate these methods; concrete plugins used by the tests fill the gap here." so future readers aren't confused.
There was a problem hiding this comment.
Rephrased in 143ae96 exactly along those lines — the comment now says AbstractPluginMessageStore does not delegate these methods, so concrete plugins used by the tests fill the gap here.
| messageStore = pluginMessageStore; | ||
| } catch (Throwable e) { | ||
| throw new RuntimeException("Initialize plugin's class: " + pluginClass + " not found!", e); | ||
| throw new RuntimeException("Failed to initialize message store plugin: " + pluginClass, e); |
There was a problem hiding this comment.
Optional: since the original message hinted at the classpath, it may help operators to log at ERROR before throwing so that the cause chain shows up in the broker log even if the RuntimeException is later re-wrapped. A one-liner via org.apache.rocketmq.logging.org.slf4j.Logger would be consistent with other classes in this package.
There was a problem hiding this comment.
Applied in 143ae96 — build now logs at ERROR via the store logger (LoggerName.STORE_LOGGER_NAME, org.apache.rocketmq.logging.org.slf4j) before rethrowing, so the cause chain lands in the broker log even when the RuntimeException is re-wrapped upstream.
|
Thanks @utafrali for the review! All four suggestions are addressed in 143ae96 (file mode 100644, |
…s class-not-found MessageStoreFactory.build wrapped Class.forName, getConstructor and newInstance in one catch and always reported "Initialize plugin's class: X not found!", even when the class existed but its constructor threw, so the top-level message pointed operators at classpath problems instead of the real failure. Use a neutral "Failed to initialize message store plugin" message, keep the original cause, and add unit tests covering the missing-class, throwing-constructor, chaining and no-plugin paths.
- commit MessageStoreFactoryTest with file mode 100644 like the rest of store tests
- replace try/fail/catch with assertThatThrownBy chains in the two failure tests
- rephrase the TestPlugin javadoc ("this branch" read like the PR branch)
- log plugin init failure at ERROR (store logger) before rethrowing, so the
cause chain reaches the broker log even if the RuntimeException is re-wrapped
143ae96 to
327a2b6
Compare
|
Apologies for the push noise — I rewrote the two commits to strip some stray boilerplate lines that had crept into the commit messages. No code change at all (the diff is byte-identical to what you reviewed). @RongtongJin sorry to ask for one more click — could you re-approve when convenient? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #11303 +/- ##
==========================================
Coverage 49.74% 49.75%
- Complexity 14352 14377 +25
==========================================
Files 1390 1390
Lines 103190 103266 +76
Branches 13493 13504 +11
==========================================
+ Hits 51337 51377 +40
- Misses 45688 45693 +5
- Partials 6165 6196 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4 participants
Which Issue(s) This PR Fixes
Fixes #11284
Brief Description
MessageStoreFactory.buildwrappedClass.forName,getConstructorandnewInstancein a singlecatch (Throwable)and always reportedInitialize plugin's class: X not found!— even when the plugin class existed and loaded fine but its constructor threw. Operators were pointed at classpath problems instead of the real failure. The message is now the neutralFailed to initialize message store plugin: <class>(original cause still preserved).Adds
MessageStoreFactoryTestwith 6 tests: missing class (cause chain), throwing constructor (regression: message must NOT claim "not found"), successful single-plugin build, two-plugin chaining in config order, and null/blank plugin config returning the original store.One-line production change; everything else is the new test file.
How Did You Test This Change?
mvn -B -pl store -am -Dtest=MessageStoreFactoryTest -Dsurefire.failIfNoSpecifiedTests=false test→ BUILD SUCCESS,Tests run: 6, Failures: 0, Errors: 0, Skipped: 0. The throwing-constructor test asserts the new message text and that "not found" is gone while the class exists.