Skip to content
This repository was archived by the owner on Aug 2, 2022. It is now read-only.

Move the index mapping definitions to standalone json files #526

Merged

Conversation

pakio
Copy link
Contributor

@pakio pakio commented Jun 18, 2020

Issue #, if available: #138

Description of changes:
Move the test index mapping definition, which directly written in code, to standalone json files.

Name of these json files are made referring to the function name.
I referred this code to load each json file placed in `resources/mappings/**'

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

" }"+
" }" +
"}";
String mappingFile = "mappings/phrase_index_mapping.json";
Copy link
Contributor

Choose a reason for hiding this comment

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

all the method have almost same implementation, the only different is mappingFile name, could you simply it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review! I made a function to make logic and error handling common.
Also found fileToString method in TestUtils, so i stopped using Resources and used that method.

@penghuo
Copy link
Contributor

penghuo commented Jun 19, 2020

Thanks for the change.
Could you sumbit the PR against the develop branch? We plan to make master stable only for release purpose. All the new feature change will push to develop branch.

@pakio pakio changed the base branch from master to develop June 19, 2020 23:14
@pakio pakio force-pushed the split-index-mapping branch from d2e7491 to b73ad70 Compare June 19, 2020 23:39
@pakio
Copy link
Contributor Author

pakio commented Jun 19, 2020

Sure, But I found a lot of differences between master branch and develop branch, and there are three different TestUtils (but same code) at develop branch.
Should I apply same changes to all file?

@pakio pakio requested a review from penghuo June 20, 2020 00:09
@chloe-zh
Copy link
Member

Sure, But I found a lot of differences between master branch and develop branch, and there are three different TestUtils (but same code) at develop branch.
Should I apply same changes to all file?

Thanks for the changes! We are migrating the SQL codebase to a new architecture so there are still some pending changes, sorry to make you confused. We will clean up redundant codelines/files in the future.

For the index mapping methods, only TestUtils in integ-modul under src/test/java/../legacy/ is called so it's fine to apply the changes only to this file.

But it looks like you are making changes in the one under src/test/java/../util/ for now so it's actually skip the mapping methods you added. Could you move the changes toward integ-test/src/test/java/../legacy/TestUtils please?

@pakio
Copy link
Contributor Author

pakio commented Jun 20, 2020

Thank you for the detailed information! Now I understood what should I do!
I've moved every my changes which was applied to legacy/.. and index definition files into under integ -test/src/test/...

Copy link
Member

@chloe-zh chloe-zh left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

@chloe-zh chloe-zh requested a review from dai-chen June 22, 2020 19:08
Copy link
Member

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! As what I see, we're just adding test index mapping to new JSON files. So we need to make our test util start using these files in future?

@pakio
Copy link
Contributor Author

pakio commented Jun 22, 2020

@dai-chen Thanks for reviewing! I've already changed code at TestUtils.java to use these new json files. Is that enough change? or if it's not, I can make more change as needed !

@dai-chen
Copy link
Member

@dai-chen Thanks for reviewing! I've already changed code at TestUtils.java to use these new json files. Is that enough change? or if it's not, I can make more change as needed !

I may miss something but I only see the TestUtils class in legacy module is removed?

@chloe-zh
Copy link
Member

@dai-chen Thanks for reviewing! I've already changed code at TestUtils.java to use these new json files. Is that enough change? or if it's not, I can make more change as needed !

I may miss something but I only see the TestUtils class in legacy module is removed?

Hi Chen @dai-chen , there are too many code lines removed in TestUtils so it doesn’t show up by default. I guess that’s what confused you?

@dai-chen
Copy link
Member

@dai-chen Thanks for reviewing! I've already changed code at TestUtils.java to use these new json files. Is that enough change? or if it's not, I can make more change as needed !

I may miss something but I only see the TestUtils class in legacy module is removed?

Hi Chen @dai-chen , there are too many code lines removed in TestUtils so it doesn’t show up by default. I guess that’s what confused you?

Yeah, I missed that and thought the whole file was removed :) Thanks!

@chloe-zh chloe-zh merged commit d42dcbd into opendistro-for-elasticsearch:develop Jun 23, 2020
@pakio pakio deleted the split-index-mapping branch June 23, 2020 22:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants