Skip to content

Deprecate & remove serializeTargetValueType #58135

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

Open
not-napoleon opened this issue Jun 15, 2020 · 5 comments
Open

Deprecate & remove serializeTargetValueType #58135

not-napoleon opened this issue Jun 15, 2020 · 5 comments
Labels
:Analytics/Aggregations Aggregations >refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >tech debt

Comments

@not-napoleon
Copy link
Member

In #42949 we removed all use of target value type in the subclasses of ValuesSourceAggregationBuilder, but left the serialization format sending a hard-coded null to preserve compatibility. This ticket tracks the work of removing that null from the serialization format, and the associated logic in VSAB.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@elasticmachine elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jun 15, 2020
@kaliljoao
Copy link

Hi guys, I would like to work on this issue.

@not-napoleon
Copy link
Member Author

@kaliljoao Sounds good, let me know if you need any help! (Sorry for the slow reply - I was out of office last week)

@kaliljoao
Copy link

@kaliljoao Sounds good, let me know if you need any help! (Sorry for the slow reply - I was out of office last week)

Hey @not-napoleon, can U help me? I started my work on this issue today and I am confused about what exactly I need to do. Do I need only to remove where null was hard-coded? My doubt was caused because it is just one line of code, if I am not wrong of course.

kaliljoao added a commit to kaliljoao/elasticsearch that referenced this issue Dec 8, 2020
kaliljoao added a commit to kaliljoao/elasticsearch that referenced this issue Dec 8, 2020
@not-napoleon
Copy link
Member Author

@kaliljoao You're right, it is only a couple of lines of code. The difficulty is that we need to maintain backwards compatibility and carefully deprecate this method. You need to detect the target version (we have some tools for this) and adapt the wire format accordingly. Honestly, this is a tricky issue and not one I'd really recommend for someone not well versed in how Elasticsearch maintains backwards compatibility. Usually we don't expect community members to deal with that, which is why this wasn't labeled help wanted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations >refactoring Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >tech debt
Projects
None yet
Development

No branches or pull requests

4 participants