Repository navigation
camel-influxdb2: deprecate retentionPolicy, which has no effect - #29709
Dhairya-007 wants to merge 3 commits into
Conversation
gnodet-bot
left a comment
There was a problem hiding this comment.
The @Deprecated annotation is correctly added to the private retentionPolicy field, but the public accessor methods getRetentionPolicy() and setRetentionPolicy() are not in this diff — meaning they remain unannotated.
In Java, @Deprecated on a private field does not propagate to its getters/setters. Any caller using endpoint.setRetentionPolicy(...) will receive no compiler deprecation warning, which defeats the purpose of the annotation.
Please add @Deprecated (and ideally a matching @deprecated Javadoc tag) to both accessor methods as well.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| @Metadata(deprecationNote = "Never read by the component: the InfluxDB 2.x write API has no retention policy," | ||
| + " retention is a property of the bucket") | ||
| @Deprecated | ||
| private String retentionPolicy = "default"; |
There was a problem hiding this comment.
@Deprecated is placed on the private field, but the public accessors getRetentionPolicy() / setRetentionPolicy() are not in this diff and will remain unannotated. Java's @Deprecated does not propagate from a field to its getters/setters — callers won't see a compiler warning.
Please add @Deprecated (and a @deprecated Javadoc tag) to both methods.
gnodet-bot
left a comment
There was a problem hiding this comment.
@Deprecated is placed on the private field, but the public accessors getRetentionPolicy() / setRetentionPolicy() are not in this diff and will remain unannotated. Java's @Deprecated does not propagate from a field to its getters/setters — callers won't see a compiler warning.
Please add @Deprecated (and a @deprecated Javadoc tag) to both methods.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
| @Metadata(deprecationNote = "Never read by the component: the InfluxDB 2.x write API has no retention policy," | ||
| + " retention is a property of the bucket") | ||
| @Deprecated | ||
| private String retentionPolicy = "default"; |
There was a problem hiding this comment.
@Deprecated is placed on the private field, but the public accessors getRetentionPolicy() / setRetentionPolicy() are not in this diff and will remain unannotated. Java's @Deprecated does not propagate from a field to its getters/setters — callers won't see a compiler warning.
Please add @Deprecated (and a @deprecated Javadoc tag) to both methods.
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR: the analysis is right, retentionPolicy and camelInfluxDB.RetentionPolicy are never read; one convention gap is left before this is ready.
I confirmed that in camel-influxdb2 the option and the header are only referenced by their own declaration and accessors; InfluxDb2Producer never reads them, so deprecating them with a deprecationNote and an upgrade-guide entry is the right treatment. The generated JSON follows the shape the schema generator emits ("... Deprecation note: ...").
One thing to add, the same point the bot raised: getRetentionPolicy() / setRetentionPolicy() in InfluxDb2Endpoint.java (lines 141 and 145) are not deprecated. The precedent you followed, apiKeyHeader in DoclingConfiguration (#29591), puts @Deprecated and a @deprecated Javadoc tag on both the getter and the setter, so Java callers get the compiler warning too:
/**
* @deprecated never read: the InfluxDB 2.x write API has no retention policy, retention is a property of the bucket
*/
@Deprecated
public String getRetentionPolicy() {(same for the setter). Heads-up: #29706 appends an upgrade-guide entry at the same spot, so one of the two will need a small rebase once the other merges.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
gnodet-bot
left a comment
There was a problem hiding this comment.
The regen commit adds @Deprecated to the generated DSL builder methods — that part is correct.
The remaining gap: getRetentionPolicy() and setRetentionPolicy() in InfluxDb2Endpoint.java (lines 141 and 145 in the current file) are still not annotated. @Deprecated on the private field does not propagate to the accessor methods — Java callers who use the endpoint API directly will not receive a compiler warning.
As noted in the existing reviews (gnodet-bot and @davsclaus), please add @Deprecated and a @deprecated Javadoc tag to both accessors:
/**
* @deprecated Never read by the component: the InfluxDB 2.x write API has no retention policy,
* retention is a property of the bucket.
*/
@Deprecated
public String getRetentionPolicy() {
return retentionPolicy;
}
/**
* @deprecated Never read by the component: the InfluxDB 2.x write API has no retention policy,
* retention is a property of the bucket.
*/
@Deprecated
public void setRetentionPolicy(String retentionPolicy) {
this.retentionPolicy = retentionPolicy;
}Everything else in this PR — the field annotation, the constant deprecation, the upgrade guide entry, the generated catalog JSON, and the DSL builder — looks correct.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
davsclaus
left a comment
There was a problem hiding this comment.
TL;DR: thanks for the regen, it is exactly what CI produced (formatter alignment plus @Deprecated on the generated influxdbRetentionpolicy() header method). Two things are left: the accessors, and a rebase.
[Important] The accessors are still not deprecated (InfluxDb2Endpoint.java:141 and :145): getRetentionPolicy() and setRetentionPolicy() have neither @Deprecated nor a @deprecated Javadoc tag, so Java callers get no compiler warning. This is the point from my earlier review and from gnodet-bot's two threads; the apiKeyHeader precedent in DoclingConfiguration (#29591) shows the shape:
/**
* @deprecated never read: the InfluxDB 2.x write API has no retention policy, retention is a property of the bucket
*/
@Deprecated
public String getRetentionPolicy() {(and the same for the setter).
[Important] Merge conflict in the upgrade guide: main has since appended several entries at the end of camel-4x-upgrade-guide-4_24.adoc, so the PR no longer merges cleanly. A rebase on main fixes it; keep your section after the last one. The other files merge on their own (main only corrected the adjacent operation description in #29646), but rebuilding the module after the rebase makes sure the generated files stay in sync.
[Minor] Optional, since you will touch the file anyway: replacing the description with "(deprecated) Never read: ..." while the deprecationNote also says "Never read by the component: ..." makes the generated text say it twice ("(deprecated) Never read: ... Deprecation note: Never read by the component: ..."). The docling precedent keeps the original description and puts the explanation only in the deprecationNote; the catalog already marks the option and the header as deprecated.
Claude Code on behalf of davsclaus. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
The retentionPolicy endpoint option and the camelInfluxDB.RetentionPolicy header are both declared and documented, and nothing reads either of them. A retention policy is an InfluxDB 1.x concept: its write API takes one per write, and camel-influxdb does pass it, from the same header name. The InfluxDB 2.x API this component calls has no such parameter - retention belongs to the bucket - so InfluxDb2Producer never asks for either value. The header even kept the 1.x spelling, camelInfluxDB.RetentionPolicy, while every other header of this component is CamelInfluxDB2..., which is what gives the copy away. Deprecate both with a note, as camel-aws2-s3vectors' unused headers and camel-tensorflow-serving's target header are deprecated, and say in the description that they are never read. The catalog metadata, the endpoint DSL builder and the 4.24 upgrade guide follow.
Review follow-up on apache#29709. @deprecated on the private field does not reach getRetentionPolicy and setRetentionPolicy, so a caller using the endpoint API directly got no compiler warning. Both now carry @deprecated and a @deprecated tag, as DoclingConfiguration's apiKeyHeader does. The descriptions go back to their original text. Replacing them with "(deprecated) Never read: ..." while the deprecationNote also says "Never read by the component: ..." made the generated text say it twice; the docling precedent keeps the description and puts the explanation only in the note, and the catalog already marks both as deprecated. Both JSON copies and the two DSL javadoc blocks follow.
f2a364b to
922728f
Compare
|
All three points are in, thanks — and sorry it took a second pass on the accessors; the bot raised it twice and I missed that The accessors are deprecated. /**
* @deprecated Never read by the component: the InfluxDB 2.x write API has no retention policy, retention is a
* property of the bucket
*/
@Deprecated
public String getRetentionPolicy() {Rebased on main. The only conflict was the one you predicted, in The doubled wording is gone — good catch, it read badly. Both descriptions go back to their original text ("Define the retention policy to the data created by the endpoint." and "The string that defines the retention policy to the data created by the endpoint"), so the generated text is now the docling shape: the original description plus The JIRA — still no credentials for issues.apache.org here; a committer creating the ticket (or giving me the number) is all I need to retitle and amend. |
gnodet-bot
left a comment
There was a problem hiding this comment.
Following up on my earlier review — all previous findings have been addressed.
- ✅
getRetentionPolicy()andsetRetentionPolicy()now carry@Deprecatedand a@deprecatedJavadoc tag — Java callers will get the compiler warning. - ✅ Description restored to original text; deprecation note appears once in the generated JSON (matching the tensorflow-serving precedent).
- ✅ Rebase on main resolved cleanly.
LGTM.
This review was generated by an AI agent, Hermès on behalf of @gnodet.
oscerd
left a comment
There was a problem hiding this comment.
Clean, complete deprecation.
The rationale holds: retentionPolicy and the camelInfluxDB.RetentionPolicy header are an InfluxDB 1.x idea (its write API takes a retention policy per write, and camel-influxdb does pass it), but the 2.x write API has none, retention being a property of the bucket, and InfluxDb2Producer never reads either. Deprecating both the option and the header with a note that says exactly that is right.
The regeneration is complete: deprecated: true plus the deprecationNote land in both the component META-INF JSON and the camel-catalog copy, and the endpoint DSL factory carries it too. @Deprecated is on the field, the getter, the setter and the RETENTION_POLICY constant, and the upgrade guide has an entry. The getter and setter each carry a single @deprecated javadoc block, so the generated docs stay clean.
Good.
Claude Code on behalf of oscerd
Problem
camel-influxdb2declares and documents two things that nothing reads:retentionPolicyendpoint option (InfluxDb2Endpoint, defaultdefault), andcamelInfluxDB.RetentionPolicyheader (InfluxDb2Constants.RETENTION_POLICY, carrying a@Metadatadescription).Across the whole module, the only references to either are their own declaration, getter and setter.
InfluxDb2Producernever asks for them.A retention policy is an InfluxDB 1.x concept: its write API takes one per write, and
camel-influxdbreally does use it —— while the InfluxDB 2.x write API this component calls has no such parameter. Retention is a property of the bucket.
The header even kept the 1.x spelling,
camelInfluxDB.RetentionPolicy, while every other header in this component isCamelInfluxDB2…(CamelInfluxDB2MeasurementName,CamelInfluxDB2WritePrecision, …). That is what gives the copy away.A user who sets either one gets no warning and no effect, and the option's default of
defaultmakes it look like a working setting.Fix
Deprecate both with a
deprecationNote, and say in the description that they are never read — the same treatmentcamel-aws2-s3vectorsgave its unusedSIMILARITY_THRESHOLD/SIMILARITY_SCOREheaders andcamel-tensorflow-servinggave itstargetheader.Changed:
InfluxDb2EndpointandInfluxDb2Constants—@Deprecated+deprecationNote, and a description that says what is going on.influxdb2.jsonand itscamel-catalogcopy —"deprecated": trueplus the note, for both the parameter and the header. The two files are byte-identical afterwards.InfluxDb2EndpointBuilderFactory—@Deprecatedand the re-wrapped javadoc.camel-4x-upgrade-guide-4_24.adoc— the entry a deprecation takes, pointing users at the bucket's retention period.The generated files were updated by hand, since I cannot run maven here; I took the shape from the existing deprecated headers and re-wrapped the DSL javadoc at the generator's own width (69 characters of text at indent 9), checking first that the wrap reproduces the untouched lines. If the regen job still finds a difference I will take its patch.
Tests
No behaviour changes — the option and header did nothing before and do nothing now, they are only labelled honestly — so there is nothing new to assert. The existing
camel-influxdb2tests cover the writes, which this does not touch.