Skip to content

Conversation

@pgomulka
Copy link
Contributor

retrofits typed endpoint and type in request parsing
the original types removal commit
#46983

relates #51816

  • Have you signed the contributor license agreement?
  • Have you followed the contributor guidelines?
  • If submitting code, have you built your formula locally prior to submission with gradle check?
  • If submitting code, is your pull request against master? Unless there is a good reason otherwise, we prefer pull requests against master and will backport as needed.
  • If submitting code, have you checked that your submission is for an OS and architecture that we support?
  • If you are submitting this code for a class then read our policy for that.

retrofits typed endpoint and type in request parsing
the original types removal commit
elastic#46983

relates elastic#51816
@pgomulka pgomulka added :Core/Infra/REST API REST infrastructure and utilities v8.0.0 labels May 31, 2021
@pgomulka pgomulka self-assigned this May 31, 2021
@elasticmachine elasticmachine added the Team:Core/Infra Meta label for core/infra team label May 31, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@pgomulka pgomulka marked this pull request as draft May 31, 2021 17:36
ensureOpen();
bulkRequest.add(data, defaultIndex, null, null, defaultPipeline, null,
true, xContentType);
true, xContentType, RestApiVersion.current()); //TOASKONREVIEWis this only used in server, not from Rest?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

can BulkProcessor be used from RestApi? I could not find usages. Should it support compatible API?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

from what we discussed on compatible-rest-api sync, BulkProcessor is not called from Rest layer, so should not need to care about the version from the RestRequest.

@pgomulka pgomulka marked this pull request as ready for review June 1, 2021 13:38
Copy link
Contributor

@romseygeek romseygeek left a comment

Choose a reason for hiding this comment

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

Some styling nits but LGTM otherwise

apply plugin: 'elasticsearch.yaml-rest-test'
apply plugin: 'elasticsearch.internal-cluster-test'
apply plugin: 'elasticsearch.internal-test-artifact-base'
//apply plugin: 'elasticsearch.yaml-rest-compat-test'
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not necessary?

'bulk/51_refresh_with_types/refresh=true immediately makes changes are visible in search',
'bulk/51_refresh_with_types/refresh=wait_for waits until changes are visible in search',
'bulk/81_cas_with_types/Compare And Swap Sequence Numbers',
// 'bulk/11_basic_with_types/Array of objects',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be deleted rather than commented out?

builder.field(STATUS, response.status().getStatus());
} else {
builder.field(_INDEX, failure.getIndex());
if(builder.getRestApiVersion() == RestApiVersion.V_7){
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spacing

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.field(INDEX_FIELD, index);
if(builder.getRestApiVersion() == RestApiVersion.V_7){
Copy link
Contributor

Choose a reason for hiding this comment

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

spacing here too

* Helper to parse bulk requests. This should be considered an internal class.
*/
public final class BulkRequestParser {
private static final DeprecationLogger deprecationLogger = DeprecationLogger.getLogger(BulkRequestParser.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: extra space?

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

Labels

:Core/Infra/REST API REST infrastructure and utilities >enhancement Team:Core/Infra Meta label for core/infra team v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants