-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Support Infinispan Client in Protean #667
Conversation
hotDeployment.produce(new HotDeploymentConfigFileBuildItem(HOTROD_CLIENT_PROPERTIES)); | ||
|
||
ClassLoader cl = Thread.currentThread().getContextClassLoader(); | ||
InputStream stream = FileLookupFactory.newInstance().lookupFile(HOTROD_CLIENT_PROPERTIES, cl); |
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.
just nitpicking, but FYI in the scope of this project there are no modular classloaders or similar things. The preference is to go for fast & light.. so rather than having a FileLookup
strategy with multiple possible places to look for resources, you might as well just load whatever you need straight away in the most simple way.
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, let me try to pair this down.
))); | ||
|
||
@BuildStep | ||
UnremovableBeanBuildItem ensureBeanLookupAvailible2() { |
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.
multiple typos in method name?
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.
Whoops, copied from somewhere without looking too closely :)
@BuildStep | ||
UnremovableBeanBuildItem ensureBeanLookupAvailible2() { | ||
return new UnremovableBeanBuildItem(beanInfo -> { | ||
Set<Type> types = beanInfo.getTypes(); |
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.
I'm a bit lost on this. Why not just use org.jboss.shamrock.arc.deployment.UnremovableBeanBuildItem.BeanClassNameExclusion
?
Remember we still want to be able to bootstrap in JVM mode in ~100 ms, a lot of that relates to the style of the codebase.
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.
That didn't work when I had tried it (unless I messed something up), @stuartwdouglas recommended this way. I can look again though.
private BeanManager beanManager; | ||
|
||
private void initialize() { | ||
log.info("Intializing CacheManager"); |
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.
typo in the INFO message :)
BTW probably shouldn't be INFO? we're trying to boot most things quite siltently. Of course we're not there yet as there's a lot of default noise coming from existing old code, but let's try to get there eventually.
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.
Sorry this should probably be removed - this was more when I was testing.
String marshallerClassName = (String) properties.get(ConfigurationProperties.MARSHALLER); | ||
if (marshallerClassName != null) { | ||
Class<?> marshallerClass = Class.forName(marshallerClassName); | ||
properties.put(ConfigurationProperties.MARSHALLER, Util.getInstance(marshallerClass)); |
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.
Similar to my previous comment about FileLookupFactory
, Util.getInstance
is probably overkill for what you really need?
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.
Yeah shouldn't it seems like.
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.
Looking at it closer, we need this invocation as is.
@@ -0,0 +1,47 @@ | |||
package org.infinispan.protean.runtime; |
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.
move to /substitutions package?
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.
I was going to the problem was I was referencing a package private variable - I will probably just change that though.
|
||
/** | ||
* Class that has all the query substitutions necessary to remove code that is loaded when proto marshaller is in use | ||
* Note this class is in the runtime package to reference package protected class name |
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.
I'm guessing this comment pre-dates the moment you figured all substitutions need being in the runtime package?
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.
It was meant more to explain why it wasn't in the runtime/substituions package. But I will get it moved over.
@@ -0,0 +1,18 @@ | |||
target | |||
*.iml |
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.
isn't it odd to have various .gitignore
files not in the root?
I've never seen this. Is it intentional?
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.
I was doing it more for supporting files I was using in testing *.jks etc. I think it would be better to add these as excludes in the parent as you mentioned though.
you can now do `jbang app install --native yourapp.java` and `yourapp` will be in your path and be built/run natively.
Add in support infinispan remote client dependent on 10.0.0.Alpha3.
All tests in both JVM and native mode pass.
As mentioned in README, the following features are working properly:
Includes named caches via @Remote annotation
Note this build requires rc11 of graal now.