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

Jump to bottom

ISPN-7714 Extend server configuration to allow easier deployment of user code #5535

Merged
merged 4 commits into from Oct 27, 2017
Merged

ISPN-7714 Extend server configuration to allow easier deployment of user code #5535

merged 4 commits into from Oct 27, 2017

Conversation

anistor
Copy link
Member

@anistor anistor commented Oct 25, 2017

In this particular case it is just the analysers, but it can be anything else using classpath discovery via java.util.ServiceLoader mechanism or another configuration file.

https://issues.jboss.org/browse/ISPN-7714

@anistor anistor added this to the 9.2.0.Alpha2 milestone Oct 25, 2017
@anistor anistor mentioned this pull request Oct 25, 2017
Closed
@anistor anistor force-pushed the t_7714_analyzer_server_deployment_m branch 6 times, most recently from 52f3b66 to f1e5966 Compare October 25, 2017 14:45
wburns
wburns requested changes Oct 25, 2017
View reviewed changes
Copy link
Member

@wburns wburns left a comment

Choose a reason for hiding this comment

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

Just some minor things. Tbh I am not that familiar with this code, if someone else could review :)

Also it seems the new test failed a couple runs ago http://ci.infinispan.org/job/Infinispan/job/PR-5535/2/

...pan/src/main/java/org/jboss/as/clustering/infinispan/subsystem/CacheContainerAddHandler.java Outdated

if (model.hasDefined(ModelKeys.MODULES) && model.get(ModelKeys.MODULES).hasDefined(ModelKeys.MODULES_NAME)) {
ModelNode modulesModel = model.get(ModelKeys.MODULES, ModelKeys.MODULES_NAME);
LinkedList<ModuleIdentifier> modules = new LinkedList<>();
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this is a LinkedList? We could just initialize this as an ArrayList with size equal to the size of the list returned from asList.

Copy link
Member Author

Choose a reason for hiding this comment

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

no particular reason. suggestion applied.

...in/java/org/jboss/as/clustering/infinispan/subsystem/CacheContainerConfigurationBuilder.java Outdated
@@ -116,6 +120,18 @@ public ServiceName getServiceName() {
.addDependency(ScheduledThreadPoolResource.EXPIRATION.getServiceName(this.name), ThreadPoolConfiguration.class, this.expirationThreadPool)
.addDependency(ScheduledThreadPoolResource.REPLICATION_QUEUE.getServiceName(this.name), ThreadPoolConfiguration.class, this.replicationQueueThreadPool)
;
if (module != null) {
if (!module.getName().equals("org.infinispan.extension")) {
// todo [anistor] only works for dynamic modules
Copy link
Member

Choose a reason for hiding this comment

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

I noticed these similar todos around. Is this something we need to do in this PR or do we need a different JIRA?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this should be a different JIRA, not reported yet. Will report it and update the comments with the id.

...in/java/org/jboss/as/clustering/infinispan/subsystem/CacheContainerConfigurationBuilder.java Outdated
@@ -229,11 +245,34 @@ public GlobalConfiguration getValue() {
return builder.build();
}

private ClassLoader makeAggregatedClassLoader(ModuleLoader moduleLoader, ModuleIdentifier cacheContainerModule, List<ModuleIdentifier> additionalModules) throws ModuleLoadException {
if (additionalModules != null) {
List<ClassLoader> classLoaders = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

We could make this a ClassLoader[] that is initialized to additionalModules.size +1 if cacheContainerModule wasn't null.

Copy link
Member Author

Choose a reason for hiding this comment

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

Both could be null. Creating a list that will contain module + modules will not look pretty, so I preferred to pass both module and modules as separate args to makeAggregatedClassLoader.

Copy link
Member

Choose a reason for hiding this comment

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

additionalModules in this section was already checked against null. But I am not picky about this, I am fine the way it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not understand what you mean.

...src/main/java/org/jboss/as/clustering/infinispan/subsystem/InfinispanSubsystemXMLReader.java Outdated
@@ -245,6 +245,14 @@ private void parseContainer(XMLExtendedStreamReader reader, PathAddress subsyste
}
break;
}
case MODULES: {
if (namespace.since(Namespace.INFINISPAN_SERVER_9_1)) {
Copy link
Member

Choose a reason for hiding this comment

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

Should be 9_2 right?

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed. solved.

@anistor anistor force-pushed the t_7714_analyzer_server_deployment_m branch from f1e5966 to d05bd28 Compare October 25, 2017 15:05
@anistor
Copy link
Member Author

anistor commented Oct 25, 2017

The CI failures should be gone soon. I've pushed the fix.

@anistor anistor force-pushed the t_7714_analyzer_server_deployment_m branch 2 times, most recently from 75a4f80 to 0a285d0 Compare October 25, 2017 17:07
@wburns
Copy link
Member

wburns commented Oct 25, 2017

It looks like this is having issues with finishing the build

http://ci.infinispan.org/job/Infinispan/job/PR-5535/8/console

Postponing this until next build.

@wburns wburns modified the milestones: 9.2.0.Alpha2, 9.2.0.Beta1 Oct 25, 2017
@wburns
Copy link
Member

wburns commented Oct 25, 2017

It looks like the failures are no on master as well, so the issue seems unrelated.

@anistor anistor force-pushed the t_7714_analyzer_server_deployment_m branch 2 times, most recently from 867be83 to 08226b6 Compare October 26, 2017 13:53
@wburns wburns modified the milestones: 9.2.0.Beta1, 9.2.0.Alpha2 Oct 26, 2017
@wburns
Copy link
Member

wburns commented Oct 26, 2017

This needs rebase so it can pass.

@anistor anistor force-pushed the t_7714_analyzer_server_deployment_m branch from 08226b6 to ecda69b Compare October 26, 2017 14:54
@anistor
Copy link
Member Author

anistor commented Oct 26, 2017

rebased

@anistor anistor force-pushed the t_7714_analyzer_server_deployment_m branch 3 times, most recently from 605e05a to d567102 Compare October 26, 2017 16:40
@anistor
Copy link
Member Author

anistor commented Oct 26, 2017

I have updates some test again.

@anistor
ISPN-7714 Avoid usage of Class.getSimpleName() in favor of Class.getN…
aaa06ec 
…ame()

* getSimpleName() requires access to all outer classes of the service class in question,
  which might not be available when the service is an inner class defined in an
  arquillian test suite and this may result in java.lang.NoClassDefFoundError
@anistor
ISPN-7714 Add more debug logging
868ffdf 
Add more debug logging
@anistor anistor force-pushed the t_7714_analyzer_server_deployment_m branch from d567102 to e525044 Compare October 27, 2017 10:54
@anistor
ISPN-7714 Configure the global classloader of a CacheManager from a s…
435f3a7 
…et of modules in server

* add a 'modules' element under the cache container config element allowing the user to list
  all the extra modules to add to the global ClassLoader of this cache container
* lookup Hibernate Search entities in the global classloader
@anistor
ISPN-7714 Test full-text queries in server mode
0cf6a49
@anistor anistor force-pushed the t_7714_analyzer_server_deployment_m branch from e525044 to 0cf6a49 Compare October 27, 2017 12:51
@gustavocoding gustavocoding merged commit 66fc0b5 into infinispan:master Oct 27, 2017
@gustavocoding
Copy link

Integrated, thanks @anistor

@anistor anistor deleted the t_7714_analyzer_server_deployment_m branch October 29, 2017 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wburns wburns wburns approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
9.2.0.Alpha2
3 participants
@anistor @wburns @gustavocoding