vlsi commented on PR #6726:
URL: https://github.com/apache/jmeter/pull/6726#issuecomment-5011504588
@e345ee first, an apology. You've now done two rounds of substantial work on
this, and neither the earlier review nor I flagged the thing that should have
been said before you started: `CSVDataSet` is a `TestBean`, and the split you
set out to fix is the TestBean design rather than a defect in this class.
`TestBeans` keep their persistent state in the element's property map and
mirror it into plain Java fields; `TestBeanHelper.prepare()` is the sync point,
and it runs on the per-thread clone. That's why the fields are `transient` and
why `TestBeanGUI` never touches them — it reads and writes the property map
directly. So a getter returning `null` on a freshly loaded tree is the
contract, and #6451 was reading a runtime accessor at design time.
`TestBeanHelper.prepare(element)` or `getPropertyAsString("filename")` gives
the reporter what they wanted. I've replied there and I'm closing it as
works-as-designed.
That's on me for not saying so two rounds ago. It's the kind of thing a
maintainer is supposed to catch on the first pass.
## What this PR actually is now
Not a bug fix — a design proposal: move a TestBean from "POJO fields +
`prepare()`" to property-backed storage, with `PropertyBackedTestBean` as the
opt-out marker. That's a legitimate proposal, and the current revision is a
good implementation of it. It's just a much bigger question than #6451, and it
shouldn't be settled in a thread about `getFilename()`.
Worth separating two things that TestBean bundles together:
- **BeanInfo-driven GUI generation.** You write a `BeanInfo`, you get a
working GUI with no Swing code. This is TestBean's real value and nobody wants
to lose it.
- **Field-based storage with a `prepare()` sync step.** This is what causes
the reported behavior.
Your PR shows these are separable: `CSVDataSet` still implements `TestBean`,
the GUI still works, only the storage changed. So the question isn't "TestBean
or schemas" — it's whether the storage half should go.
## The case for migrating
- It removes an entire bug class rather than one instance. All 32 TestBean
elements have the same trap; JSR223 elements and `ConstantThroughputTimer`
behave identically today.
- `prepare()` is reflective and runs **per sample** for samplers
(`JMeterThread#doSampling`), invoking a write method for every declared
property whether or not the sampler reads it. Property-backed access does a map
lookup only for properties actually read. This plausibly nets out faster,
though that claim needs a benchmark before anyone leans on it.
- Schemas have existed since 5.6 and are where the codebase is going.
`Argument` and `ConfigTestElement` are already property-backed.
- The new property editors (`JBooleanPropertyEditor` and friends) bind to
schema descriptors, so TestBean elements can't currently use them or the
modified-value gutter.
- Defaults stop living in two places. Your `CSVDataSetBeanInfo` change,
reading `DEFAULT` from the schema descriptors, is a good demonstration.
- 32 elements is a tractable number, and most are smaller than `CSVDataSet`.
## The case against, or at least for caution
- Third-party plugins implement `TestBean`. The contract has to keep
working, so any migration must stay additive. `PropertyBackedTestBean` is
additive, which is right — but a codebase with two storage models half-migrated
for two years is worse than either model.
- Function evaluation timing changes. Today `prepare()` freezes `${...}`
into a field; property-backed access re-evaluates per read, with
`FunctionProperty`'s per-iteration cache. That is arguably more correct, but
it's an observable semantic change and some element may depend on the freeze.
- Getters start returning schema defaults where they returned `null`. Your
PR already had to update three existing assertions from `assertNull` to
`assertEquals("")`.
- Hot-path cost moves from "one reflective sync per sample" to "a map lookup
per read". Direction depends on access patterns; measure rather than assume.
## Suggested path
If there's appetite for this, I'd rather we decide the semantics once, up
front, than rediscover them per element:
1. Settle `null` vs schema default for unset properties, and the
function-evaluation timing, as project-wide decisions.
2. Land `PropertyBackedTestBean` on its own, with `@API(EXPERIMENTAL)` and
javadoc stating the contract. Consider requiring `getSchema()` rather than
leaving it a bare marker, so the invariant is enforced.
3. Migrate elements in small batches, `CSVDataSet` first as the reference.
I'd like to hear from other maintainers before we commit to any of that.
## For this PR meanwhile
Two things I'd change regardless of the outcome:
- The `setProperty` fast path skips normalization for values without spaces.
`shareMode.all` renders as `所有线程` in the Chinese bundle, which has no space, so
old plans saved under a CJK locale would no longer migrate to the canonical
key. The existing comment does describe a space-based shortcut, but that
comment describes an optimization that was never implemented and doesn't hold
for languages without word spacing. Comparing against the three canonical enum
values is both cheaper and locale-safe.
- With #6451 closing as works-as-designed, the `changes.xml` entry needs
rewording — this is a behavior change, not a bug fix.
To be clear about the outcome I'd like: I don't want this to end with your
work discarded. The implementation is sound and it's a good reference point for
the design discussion. I'd just rather it land as a deliberate change with
maintainer buy-in than as a fix for an issue that turned out not to be a bug.
--
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]
