Skip to content

Add parsing metadata from crud response#278

Merged
akudiyar merged 1 commit intomasterfrom
metadata
Dec 14, 2022
Merged

Add parsing metadata from crud response#278
akudiyar merged 1 commit intomasterfrom
metadata

Conversation

@ArtDu
Copy link
Copy Markdown
Contributor

@ArtDu ArtDu commented Oct 3, 2022

Now we can obtain metadata from the response and don't use metadata from ddl.

I didn't forget about:

  • Tests
  • Changelog
  • Documentation
  • Clean up the code for review. See checklist

Closes #272

@ArtDu ArtDu force-pushed the metadata branch 2 times, most recently from 0e416d1 to 7e4ceca Compare October 3, 2022 15:58
@ArtDu ArtDu marked this pull request as ready for review October 3, 2022 16:31
@ArtDu ArtDu requested review from Elishtar and akudiyar October 3, 2022 16:31
@ArtDu
Copy link
Copy Markdown
Contributor Author

ArtDu commented Oct 11, 2022

The main problem is that now we have two converters used to parse incoming tuples:
The first level parses the top level - container - List of tuples or crud response.
The second level parses one tuple at a time.

The second level always needs to know the metadata, because it creates a TarantoolTuple and passes it there. Right now it only gets the metadata from the constructor.
The conclusion is the level that parses a particular tuple must know the metadata from the level above, if it is crudResponse.

So I see 3 options:

  1. Create a new fromValue method, into which we can pass custom metadata, as I did in PR - minus that we add a method to the API, although the method is not giant terrible)
  2. Combine the first and second levels - consider combining two converters into one - minus, that everything is in one place parsing the container and the tuples themselves
  3. Create a converter for each request of the second level - minus the memory will be painfull grown

@akudiyar
Copy link
Copy Markdown
Collaborator

akudiyar commented Oct 17, 2022

I think the new converter variant (3) is the best because it doesn't allow to leak the implementation details through interfaces and leaves in place the ability to have different result mappers (e.g. for libs other than Tarantool/crud or non-tuple results).

The converter instantiation is a parallel problem and should be dealt with as a whole, not in this particular place.

Copy link
Copy Markdown
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

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

It's a better idea overall then changing the ValueConverter interface, however the changes look very rough. I'd rather not copy-paste the whole converters code in that way. Also It may be worth thinking about passing a tuple converter factory to the result converter instead of a tuple converter, that will help eliminating the fromValue method and unnecessary casts.

@ArtDu ArtDu marked this pull request as draft November 7, 2022 23:06
@ArtDu ArtDu changed the title Add parsing metadata from crud response WIP: Add parsing metadata from crud response Nov 7, 2022
@ArtDu ArtDu force-pushed the metadata branch 3 times, most recently from 6cd2683 to d39b171 Compare November 10, 2022 10:20
@ArtDu ArtDu force-pushed the metadata branch 3 times, most recently from 188c651 to 895c123 Compare November 16, 2022 20:06
@ArtDu ArtDu changed the base branch from master to refactor/mappers December 7, 2022 11:20
@ArtDu ArtDu changed the title WIP: Add parsing metadata from crud response Add parsing metadata from crud response Dec 7, 2022
Base automatically changed from refactor/mappers to master December 9, 2022 08:59
@ArtDu ArtDu marked this pull request as ready for review December 9, 2022 14:11
@ArtDu
Copy link
Copy Markdown
Contributor Author

ArtDu commented Dec 9, 2022

@akudiyar @Elishtar Can you look it again? We don't have a lot of codes here after #300 was merged

@ArtDu ArtDu requested review from Elishtar and akudiyar December 9, 2022 14:13
/**
* @author Artyom Dubinin
*/
public class CRUDResponseMetadataParser implements SpaceMetadataParser {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What's the purpose of this class? Currently it looks like doing nothing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed classes. You should check

/**
* @author Artyom Dubinin
*/
public class DDLSpaceMetadataParser implements SpaceMetadataParser {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This class needs some unit tests and a Javadoc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They're already tested in integrations tests. I don't think it's nessasary to think about syntaticly creation of ddl, _vspace, crud methods answers

/**
* @author Artyom Dubinin
*/
public interface SpaceMetadataParser {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This class needs some unit tests and a javadoc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

public class DDLSpaceMetadataParser implements SpaceMetadataParser {

@Override
public TarantoolSpaceMetadataImpl parse(Value metadata) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it take MapValue explicitly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we want to have one interface for metadata parsing we need to use Value. Metadata can be stored in different structures

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I changed it

/**
* @author Artyom Dubinin
*/
public class VSpaceMetadataParser implements SpaceMetadataParser {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This class needs unit tests and a Javadoc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@ArtDu
Copy link
Copy Markdown
Contributor Author

ArtDu commented Dec 13, 2022

We can split PR to "Parse metadata from crud response" 3252ba6 and "Refactor metadata parsing". But I prefer to merge "Parse metadata from crud response" 3252ba6 for first

Copy link
Copy Markdown
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

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

Interestingly, that looks pretty OK without all the other changes, one question is left though

* @author Artyom Dubinin
*/
class TarantoolFieldMetadataImpl implements TarantoolFieldMetadata {
public class TarantoolFieldMetadataImpl implements TarantoolFieldMetadata {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Do we really need to make it public? It's an internal implementation class.

Copy link
Copy Markdown
Contributor Author

@ArtDu ArtDu Dec 14, 2022

Choose a reason for hiding this comment

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

There are two options here:

  • package private - We store converters(parsers in future) in *.core.metadata package
  • public - we open this class and store classes where they should be semantically

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

First option has been picked up

@ArtDu ArtDu requested a review from akudiyar December 14, 2022 11:10
Copy link
Copy Markdown
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

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

LGTM

@akudiyar akudiyar merged commit 69de22c into master Dec 14, 2022
@akudiyar akudiyar deleted the metadata branch December 14, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Parse metadata from crud response

3 participants