-
Notifications
You must be signed in to change notification settings - Fork 12
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
ISPN-7342 - Cassandra Cache Store is missing Custom Cache Store service manifest #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the Pull Request! Good job!
Unfortunately I found some issues that should be addressed before integrating it.
<description>Infinispan CassandraCacheStore module</description> | ||
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | ||
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd"> | ||
<modelVersion>4.0.0</modelVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please reformat it along with our Infinispan formatter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added dependency for testng to fix compilation issue
</exclusions> | ||
</dependency> | ||
<properties> | ||
<checkstyle.skip>true</checkstyle.skip> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we skipping checkstyle?
<optional>true</optional> | ||
</dependency> | ||
|
||
<!-- https://mvnrepository.com/artifact/org.testng/testng --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment line could be removed
<version>6.10</version> | ||
</dependency> | ||
|
||
<!-- https://mvnrepository.com/artifact/org.mockito/mockito-all --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment line could be removed
<build> | ||
<!-- testResources> | ||
<testResource> | ||
<directory>src/test/resources</directory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove commented code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted changes, please review
@@ -1 +0,0 @@ | |||
org.infinispan.persistence.cassandra.configuration.CassandraStoreConfigurationParser82 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you remove the parser?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted changes, please review
…ck these once the pull request is accpted
Please check now, I have reverted few changes made as per my project requirement. |
Please check now, I have reverted few changes made as per my project requirement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check now, I have reverted few changes made as per my project requirement.
@@ -1 +0,0 @@ | |||
org.infinispan.persistence.cassandra.configuration.CassandraStoreConfigurationParser82 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted changes, please review
<build> | ||
<!-- testResources> | ||
<testResource> | ||
<directory>src/test/resources</directory> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted changes, please review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the changes. I'll fix those two missing spots and squash everything when integrating...
|
||
<dependency> | ||
<groupId>org.testng</groupId> | ||
<artifactId>testng</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be unneeded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also, there is no need to change permissions of org.infinispan.configuration.parsing.ConfigurationParser
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! Now I see it... TestNG is required...
https://issues.jboss.org/browse/ISPN-7342
Created proper /META-INF/services/org.infinispan.persistence.spi.AdvancedLoadWriteStore entry pointing to CassandraStore class.