Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate async replication #10642

Closed

Conversation

dadoonet
Copy link
Member

Follow up for PR #10171.

As we discussed #10114 (comment), we should also mark the code as deprecated so plugin developers know in advance that they need to adapt their code.

I also added a log when using replication parameter in REST APIs but as info Level. I wonder if I should better log that as a WARN if debug level is set? So people in debug mode will see this information as a WARN while it won't pollute logs for each call when in default INFO log level.

@dadoonet dadoonet self-assigned this Apr 17, 2015
@dadoonet dadoonet changed the title [java] Remove async replication Java-api: Deprecate async replication Apr 17, 2015
@javanna javanna changed the title Java-api: Deprecate async replication Java api: Deprecate async replication Apr 17, 2015
@javanna
Copy link
Member

javanna commented Apr 17, 2015

Thanks for openinig this @dadoonet , it LGTM. As for deprecation warnings, I think they should be warn, but let's hear what @clintongormley thinks. It would be great to have all of the deprecation logs from the same logger, so that they can all be enabled at once when needed, but this might require some more work and overlap with other work in progress that I'm not sure about.

@dadoonet
Copy link
Member Author

Ah! So we could create a DeprecationLogger class and allow a global activation or not. Would be super nice indeed!

@jpountz
Copy link
Contributor

jpountz commented Apr 20, 2015

@dadoonet @javanna I think we should remove deprecation warnings from this PR and instead make sure that the replication type is parsed with ParseField so that we have a single entry point if we want to log about usage of deprecated APIs.

@clintongormley
Copy link

I think the deprecation message should be a warning, and I agree with #10642 (comment) (see #8963)

@clintongormley
Copy link

@dadoonet any movement here?

@dadoonet
Copy link
Member Author

@clintongormley I was wondering if I should wait for #11033 to be merged in so I can update the deprecation notice there?

@jpountz
Copy link
Contributor

jpountz commented May 12, 2015

Hmm, actually ParsedField might not be applicable here (or in an awkward way) since we do not perform parsing, however I think this PR should use #11033 once it's in.

@s1monw
Copy link
Contributor

s1monw commented May 28, 2015

@dadoonet can you move this forward if possible?

@dadoonet
Copy link
Member Author

@s1monw Thanks for the heads up. Now that #11285 has been merged, I'm going to update the PR and use the deprecation logger. Stay tuned! :)

@dadoonet
Copy link
Member Author

Actually, the deprecation logger has been merged only in master (2.0) but is not available in 1.x.
The plan is to deprecate in docs in 1.x and use the deprecation logger in master.

@dadoonet dadoonet force-pushed the pr/10171-deprecate-replication-param branch from d61f0d2 to cbd2b6c Compare May 29, 2015 09:46
public BulkRequest replicationType(ReplicationType replicationType) {
this.replicationType = replicationType;
return this;
}

/**
Copy link
Member

Choose a reason for hiding this comment

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

maybe add also what the method does, although quite self-explaining

@javanna
Copy link
Member

javanna commented May 29, 2015

LGTM besides the tiny javadoc comment I left

@dadoonet dadoonet force-pushed the pr/10171-deprecate-replication-param branch from cbd2b6c to f2ed5d1 Compare May 29, 2015 10:24
Follow up for PR elastic#10171.

As we discussed elastic#10114 (comment), we should also mark the code as deprecated so plugin developers know in advance that they need to adapt their code.
@dadoonet dadoonet force-pushed the pr/10171-deprecate-replication-param branch from f2ed5d1 to 1493cfb Compare May 29, 2015 10:25
@dadoonet dadoonet added :Core/Infra/Transport API Transport client API and removed in progress labels May 29, 2015
@dadoonet
Copy link
Member Author

merged in 1.x with 1493cfb

@dadoonet dadoonet closed this May 29, 2015
@dadoonet dadoonet deleted the pr/10171-deprecate-replication-param branch May 29, 2015 10:31
@clintongormley clintongormley changed the title Java api: Deprecate async replication Deprecate async replication May 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants