Skip to content

modified documentation to not use DataConverter incorrectly - #867

Open
Krashcan wants to merge 1 commit into
cadence-workflow:masterfrom
Krashcan:docfix/worker
Open

modified documentation to not use DataConverter incorrectly#867
Krashcan wants to merge 1 commit into
cadence-workflow:masterfrom
Krashcan:docfix/worker

Conversation

@Krashcan

Copy link
Copy Markdown

What changed?
Documentation for Worker constructor

Why?
It incorrectly mentioned that things like DataConverter are being passed via options which is no longer the case since release v3.0.0.

How did you test it?
No actual code change done

Potential risks
Nothing, worst case my documentation itself is incorrect.

Release notes
No

Documentation Changes
No

@CLAassistant

CLAassistant commented Feb 28, 2024

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@zawadzkidiana zawadzkidiana left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the doc cleanup — agreed that Javadoc referencing a mechanism that no longer applies is worse than no comment at all, since it actively misleads whoever goes looking at it next.

Looking at the current Worker.java on master, though, this specific constructor's Javadoc has since been rewritten as part of a larger change — the constructor signature itself now takes factoryOptions, cache, enableStickyExecution, threadPoolExecutor, contextPropagators, etc., and the @param options line no longer mentions DataConverter at all. So this particular one-line fix looks like it's already been superseded and won't apply cleanly against current master anymore.

If there's still stale DataConverter-via-worker-options wording elsewhere in the docs/Javadoc that you ran into, it'd be great to get an updated PR against current master for that — happy to take a look. Otherwise this one is probably safe to close as no-longer-applicable. Thanks for taking the time to send it in either way!


Generated by Claude Code

Copy link
Copy Markdown

Thanks for catching this, @Krashcan, and sorry it sat here unreviewed for so long.

Your report was accurate. The Worker constructor's Javadoc said @param options Options (like {@link DataConverter} override) for configuring worker, but DataConverter hasn't been settable through WorkerOptions since v3.0.0 — the constructor pulls it from the client instead:

SingleWorkerOptions.newBuilder()
    .setDataConverter(client.getOptions().getDataConverter())

So the doc was pointing people at a knob that isn't on that dial, which is a genuinely misleading kind of stale comment — worse than no comment.

Current state on master

The line has since been fixed, though by a different route. The constructor Javadoc now reads:

 * @param options Options for configuring worker.

with the {@link} dropped entirely and the com.uber.cadence.converter.DataConverter import removed. The surrounding @param block was also filled out at the same time (factoryOptions, cache, enableStickyExecution, threadPoolExecutor, contextPropagators all have entries now). So the incorrect reference you flagged is gone.

For what it's worth, I think plain wording was the better landing spot than the substitution proposed here, for a reason worth noting: PollerOptions lives in com.uber.cadence.internal.worker. Linking it from a public-API Javadoc both adds an import of an internal type and generates a doc link into a package users aren't meant to depend on. Not something you could have been expected to weigh from outside the codebase — and it doesn't diminish the report, which is the part that actually mattered.

The branch also now conflicts with master (the file has moved on considerably since February 2024), so there's nothing left to rebase toward.

I'd suggest closing this as resolved. Thank you for reading the docs closely enough to notice, and for taking the trouble to open a PR rather than just working around it — small correctness fixes to public-API docs are genuinely valuable, and I'm sorry this particular one didn't get the timely response it deserved. Please don't let the delay put you off filing the next one.


Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants