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

OGM-1483 Define Infinispan Remote Protobuf schema providing the final file #1033

Closed

Conversation

Projects
None yet
5 participants
@fax4ever
Copy link
Contributor

commented May 28, 2018

@Sanne Sanne self-assigned this May 28, 2018

@fax4ever fax4ever force-pushed the fax4ever:1483-userdefined-protoschema branch from cc9c25f to e8cd04f May 29, 2018

String generatedWithoutSpaces = generated.replaceAll( "\\s+", " " ).trim();
String providedByServiceWithoutCommentsAndSpaces = providedByServiceWithoutComments.replaceAll( "\\s+", " " ).trim();

if ( !generatedWithoutSpaces.equalsIgnoreCase( providedByServiceWithoutCommentsAndSpaces ) ) {

This comment has been minimized.

Copy link
@fax4ever

fax4ever May 29, 2018

Author Contributor

@Sanne I don't like very much this solution... I'm going to figured out something better exploring Infinispan code.

This comment has been minimized.

Copy link
@Sanne

Sanne May 29, 2018

Member

Cool. I'll stop the review then. Let me know when it's done.

@Sanne Sanne removed their assignment May 29, 2018

@fax4ever fax4ever force-pushed the fax4ever:1483-userdefined-protoschema branch 2 times, most recently from b0ed3e5 to 28cd14e May 29, 2018

@fax4ever

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2018

@Sanne now I'm using the ProtoStream parser to parse the provided schema. You can continue the review on it whenever you want. Thank you and sorry so the delay, but the old version with regex had not convince me.

@@ -117,4 +119,33 @@ public void validate() {
public String getCacheConfiguration() {
return cacheConfiguration;
}

public boolean isDescribedIn(FileDescriptor fileDescriptor) {

This comment has been minimized.

Copy link
@fax4ever

fax4ever May 31, 2018

Author Contributor

All these isDescribedIn methods could have been implemented with a visitor pattern, but I've preferred to using the simple OO delegation. WDYT @Sanne?

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

The pattern that Shall Not Be Named! :D

The problem with the visitor pattern is that makes it hard to follow the logic sometimes, it could make sense in this case but, because the code is simple enough, I don't mind the approach you have used. We might refactor the code later if needed.

@@ -2409,6 +2409,12 @@ hibernate.ogm.infinispan_remote.schema_file_name::
Defines the file name of the generated Protobuf schema. Defualts to 'Hibernate_OGM_Generated_schema.proto'.
The file name must have a valid __*.proto__ extension.

hibernate.ogm.infinispan_remote.schema_override_resource::

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

Could you add an example of a possible value for this property? For example c:\Project\entities.proto

This comment has been minimized.

Copy link
@gsmet

gsmet Jun 5, 2018

Member

Let's use a Unix path instead of a Windows one :).

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

I don't mind, but I'm afraid that 90% of the developers work on Windows

This comment has been minimized.

Copy link
@gsmet

gsmet Jun 5, 2018

Member

At some point, they will deploy it on Linux, won't they? :)

I just think it's more appropriate to use a Unix path but, well, I won't fight for it if you think a Windows one is better. Note that there are also a lot of developers on MacOS these days and the Unix path works well for them too.

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

At some point, they will deploy it on Linux, won't they? :)

I like your world, let's live there :)

Anyway Unix works for me, I don't really mind

This comment has been minimized.

Copy link
@Sanne

Sanne Jun 5, 2018

Member

I would like to see some Windows examples too... but then make sure to test them ;-)

This comment has been minimized.

Copy link
@fax4ever

fax4ever Jun 6, 2018

Author Contributor

According to the java doc of ConfigurationPropertyReader class:
in case of URLs this string can either represent a class path element, an URL or a file system path
so I think that we can rely on helper, used for all URL extractions from any configuration.

This comment has been minimized.

Copy link
@fax4ever

fax4ever Jun 6, 2018

Author Contributor

What do you think about something like:

The value of the property is of string type and it can either represent a class path element, an URL or a file system path.

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 6, 2018

Member

The value of the property can either be a class path (example) , an URL (example) or a file system path (example) .

@@ -2409,6 +2409,12 @@ hibernate.ogm.infinispan_remote.schema_file_name::
Defines the file name of the generated Protobuf schema. Defualts to 'Hibernate_OGM_Generated_schema.proto'.
The file name must have a valid __*.proto__ extension.

hibernate.ogm.infinispan_remote.schema_override_resource::
It is possible to override the generated Protobuf schema, providing a user defined Protobuf schema in a resource file.
This will __not affect how entities are encoded__, so the alternative schema **must be compatible** with what would have been the generated one.

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

Can we change this to something more similar to:

Hibernate OGM will use the specified Protobuf Schema instead of the generated one. This doesn't affect how entities are encoded so the specified schema is expected to be compatible with the generated one.
This will __not affect how entities are encoded__, so the alternative schema **must be compatible** with what would have been the generated one.
In a nutshell the user can change only the comment parts of the Protobuf schema.
Anyway this can be very useful for defining **server side indexes** on caches, thus used for JPQL or Ickle queries to data store.

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

This can be used to defined server side indexes on caches used by JPQL and native Ickle queries

This comment has been minimized.

Copy link
@gsmet

gsmet Jun 6, 2018

Member

s/defined/define/

}

private FileDescriptor parseSchema() {
// this name is used only for local parsing, it could be any value

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

What do you mean with "local parsing"? Maybe it is a silly question

This comment has been minimized.

Copy link
@fax4ever

fax4ever Jun 5, 2018

Author Contributor

Protostream parsing API requires a file name as metadata of the Protobuf schema. This name may have no connection with the name that we use to register the schema to the server, it is used just for the parsing process.

This comment has been minimized.

Copy link
@anistor

anistor Jun 5, 2018

Member

The name should be one and the same, or the eventual parsing error messages will stop being useful.

This comment has been minimized.

Copy link
@fax4ever

fax4ever Jun 5, 2018

Author Contributor

@anistor thank you! you mean in case of parsing error, if I use a different name I will display the wrong name to the message log?

This comment has been minimized.

Copy link
@fax4ever

fax4ever Jun 5, 2018

Author Contributor

I'm going to use the same name

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

Mmmh ... any-name.proto doesn't seem a good name ... if it appears in the error message

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

@fax4ever see if you can write a test for that case, so that we can check the error message. Only if it's easy to do

This comment has been minimized.

Copy link
@anistor

anistor Jun 5, 2018

Member

Please use the real name

* @IndexedField(index=true, store=true)
*/
optional string subject = 3;
}

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

Missing new line

* @IndexedField(index=true, store=false)
*/
optional string name = 2;
}

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

Missing new line


message hibernate_sequences {
required int64 next_val = 2;
}

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

Missing new line

assertThat( protoSchema ).isEqualTo( expectedProtoSchema );
}
}
}

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

These tests seem allright but can you also check with a native query that the fields that are supposed to be indexed are actually indexed? Can it be done?

This comment has been minimized.

Copy link
@fax4ever

fax4ever Jun 5, 2018

Author Contributor

@DavideD I think there is no API for this kind of check

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 6, 2018

Member

Shouldn't you be able to add a native full text condition on a field that it is indexed? Otherwise it shouldn't work. Am I wrong?

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 6, 2018

Member

It doesn't matter, we can do it later

@@ -428,4 +431,14 @@ public static void enableCountersForInfinispan(@SuppressWarnings("rawtypes") Map
cfg.put( "hibernate.ogm.infinispan.configuration_resource_name", "infinispan-dist.xml" );
}

public static String readResource(URL resource) {
try ( InputStream is = resource.openStream() ) {
java.util.Scanner s = new java.util.Scanner( is ).useDelimiter( "\\A" );

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

Shouldn't you close the Scanner?
What's \\A? Can you keep it in a constant?

This comment has been minimized.

Copy link
@fax4ever

fax4ever Jun 5, 2018

Author Contributor

of course

This comment has been minimized.

Copy link
@fax4ever

fax4ever Jun 5, 2018

Author Contributor

I think that scanner method close just closes the input stream

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

Fair enough. Why the package declaration BTW?

@@ -428,4 +431,14 @@ public static void enableCountersForInfinispan(@SuppressWarnings("rawtypes") Map
cfg.put( "hibernate.ogm.infinispan.configuration_resource_name", "infinispan-dist.xml" );
}

public static String readResource(URL resource) {

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 5, 2018

Member

Could you move this method in a utility class? It seems something that we might need in the future somewhere else and you could remove it from TestHelper. I'm surprised that we already don't have one somewhere

This comment has been minimized.

Copy link
@fax4ever

fax4ever Jun 5, 2018

Author Contributor

good idea

@@ -2409,6 +2409,12 @@ hibernate.ogm.infinispan_remote.schema_file_name::
Defines the file name of the generated Protobuf schema. Defualts to 'Hibernate_OGM_Generated_schema.proto'.

This comment has been minimized.

Copy link
@gsmet

gsmet Jun 5, 2018

Member

s/Defualts/Defaults/

@fax4ever fax4ever force-pushed the fax4ever:1483-userdefined-protoschema branch from 28cd14e to 26c56d1 Jun 6, 2018

@fax4ever

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2018

Thank you for comments and suggestions. I did some changes. That anything else we can improve or fix?


public class ResourceHelper {

private static final String FILE_DELIMITER_PATTERN = "\\A";

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 6, 2018

Member

Is this a "file delimiter" or a "line delimiter"? Does it read multiple files?

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 6, 2018

Member

Are you going to change it? Otherwise I can do it before merging the PR. Let me know

This comment has been minimized.

Copy link
@fax4ever

fax4ever Jun 6, 2018

Author Contributor

not yet

This comment has been minimized.

Copy link
@DavideD

DavideD Jun 6, 2018

Member

Don't worry, I will do that

This comment has been minimized.

Copy link
@fax4ever

fax4ever Jun 6, 2018

Author Contributor

thank you

@fax4ever

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2018

@DavideD I'm going to split the commit

@DavideD

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

@DavideD I'm going to split the commit

Thanks

@fax4ever fax4ever force-pushed the fax4ever:1483-userdefined-protoschema branch from 26c56d1 to 4e20b2e Jun 6, 2018

@fax4ever

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2018

@DavideD I split the old commit and pushed with force the new ones

@fax4ever fax4ever force-pushed the fax4ever:1483-userdefined-protoschema branch from 4e20b2e to 2c1a587 Jun 7, 2018

@fax4ever fax4ever force-pushed the fax4ever:1483-userdefined-protoschema branch from 2c1a587 to 65b5f8a Jun 8, 2018

@DavideD

This comment has been minimized.

Copy link
Member

commented Jun 8, 2018

Merged #1033

I made a mistake and merged the previous version in a single commit.

@DavideD DavideD closed this Jun 8, 2018

@fax4ever fax4ever deleted the fax4ever:1483-userdefined-protoschema branch Jun 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.