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

ISPN-10460 Remove NestedAttributeSerializer #7183

Conversation

gustavocoding
Copy link

Copy link
Member

@danberindei danberindei left a comment

Choose a reason for hiding this comment

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

IMO in most cases it would be better to change the serialization format so those elements are attributes.

@gustavocoding
Copy link
Author

@danberindei I propose to handle schema and config changes in other JIRAs. This PR was to align the XSD, the builders, and the inner Attributes inside the various configs objects, all without changing API or schema. WDYT?

@danberindei
Copy link
Member

I wish I didn't dive so much in the RocksDB/SIFS locations specifics, because it distracts from the main point, and the location stuff should indeed be handled in separate JIRAs/PRs.

But think of it this way: the way you did it requires changing the programmatic configuration, the way I want it requires changing the XML configuration. I don't think any one is harder to do than the other, it's just that you have already written the code to change the programmatic configuration.

And just like you didn't deprecate the old methods in the configuration builders, you could not deprecate the old elements in the XML configuration and keep supporting them forever -- we just need the attributes to be the canonical version.

@gustavocoding
Copy link
Author

@danberindei, I can sort out the counters (deprecating from the XSD), and create JIRAs for the other configs, would it be enough to have this PR in?

@ryanemerson
Copy link
Contributor

@danberindei Are you happy with the latest changes? We should get this in for CR2.

@gustavonalle Needs rebasing on the docs.

@gustavocoding gustavocoding force-pushed the ISPN-10460_remove_nested_serializer branch from 73bc04b to 1917b8a Compare September 10, 2019 09:20
@gustavocoding
Copy link
Author

rebased

@danberindei
Copy link
Member

Sorry it took so long to reply @gustavonalle, looks great. I'll integrate once CI is done.

@gustavocoding gustavocoding force-pushed the ISPN-10460_remove_nested_serializer branch from 1917b8a to fab3ff3 Compare September 11, 2019 05:12
@gustavocoding
Copy link
Author

@danberindei There is one test failing which is CounterResourceTest but it always works for me. Is CI trying to do some clever stuff like running tests in a different way (or parallel?)

@gustavocoding
Copy link
Author

It doesn't look like it's the CI fault, or better saying, it looks like the only fault is being extremely slow and exposing some race conditions 😄
The test is failing because there's a weird random error when deleting a counter:

java.util.concurrent.CompletionException: org.infinispan.remoting.RemoteException: ISPN000217: Received exception from CounterResourceTest-NodeB-36672, see cause for remote stack trace
	at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:314)
	at java.base/java.util.concurrent.CompletableFuture.uniComposeStage(CompletableFuture.java:1113)
	at java.base/java.util.concurrent.CompletableFuture.thenCompose(CompletableFuture.java:2235)
	at org.infinispan.rest.resources.CounterResource.deleteCounter(CounterResource.java:109)
	at org.infinispan.rest.framework.impl.RestDispatcherImpl.dispatch(RestDispatcherImpl.java:57)
	at org.infinispan.rest.RestRequestHandler.handleRestRequest(RestRequestHandler.java:106)
	at org.infinispan.rest.RestRequestHandler.channelRead0(RestRequestHandler.java:63)
	at org.infinispan.rest.Http11RequestHandler.channelRead0(Http11RequestHandler.java:35)
	at org.infinispan.rest.Http11RequestHandler.channelRead0(Http11RequestHandler.java:18)
	at io.netty.channel.SimpleChannelInboundHandler.channelRead(SimpleChannelInboundHandler.java:105)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:374)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:360)
	
	at java.base/java.lang.Thread.run(Thread.java:834)
Caused by: org.infinispan.remoting.RemoteException: ISPN000217: Received exception from CounterResourceTest-NodeB-36672, see cause for remote stack trace
	at org.infinispan.remoting.transport.ResponseCollectors.wrapRemoteException(ResponseCollectors.java:28)
	at org.infinispan.remoting.transport.ValidSingleResponseCollector.withException(ValidSingleResponseCollector.java:37)
	at org.infinispan.remoting.transport.ValidSingleResponseCollector.addResponse(ValidSingleResponseCollector.java:21)
	at org.infinispan.remoting.transport.impl.SingleTargetRequest.addResponse(SingleTargetRequest.java:70)
	at org.infinispan.remoting.transport.impl.SingleTargetRequest.onResponse(SingleTargetRequest.java:40)
	at org.infinispan.remoting.transport.impl.RequestRepository.addResponse(RequestRepository.java:52)
	at org.infinispan.remoting.transport.jgroups.JGroupsTransport.processResponse(JGroupsTransport.java:1393)
	at org.infinispan.remoting.transport.jgroups.JGroupsTransport.processMessage(JGroupsTransport.java:1296)
	at org.infinispan.remoting.transport.jgroups.JGroupsTransport.access$300(JGroupsTransport.java:128)
	at org.infinispan.remoting.transport.jgroups.JGroupsTransport$ChannelCallbacks.up(JGroupsTransport.java:1441)
	at org.jgroups.JChannel.up(JChannel.java:775)
	... 1 more
	Suppressed: org.infinispan.util.logging.TraceException
		at org.infinispan.interceptors.impl.SimpleAsyncInvocationStage.get(SimpleAsyncInvocationStage.java:41)
		at org.infinispan.interceptors.impl.AsyncInterceptorChainImpl.invoke(AsyncInterceptorChainImpl.java:246)
		at org.infinispan.cache.impl.CacheImpl.executeCommandAndCommitIfNeeded(CacheImpl.java:1835)
		at org.infinispan.cache.impl.CacheImpl.remove(CacheImpl.java:676)
		at org.infinispan.cache.impl.CacheImpl.remove(CacheImpl.java:670)
		at org.infinispan.cache.impl.AbstractDelegatingCache.remove(AbstractDelegatingCache.java:454)
		at org.infinispan.cache.impl.EncoderCache.remove(EncoderCache.java:685)
		at org.infinispan.counter.impl.weak.WeakCounterImpl.removeWeakCounter(WeakCounterImpl.java:117)
		at org.infinispan.counter.impl.manager.EmbeddedCounterManager.removeCounter(EmbeddedCounterManager.java:282)
		at org.infinispan.counter.impl.manager.EmbeddedCounterManager.lambda$removeCounter$0(EmbeddedCounterManager.java:129)
		at java.base/java.util.concurrent.ConcurrentHashMap.compute(ConcurrentHashMap.java:1908)
		at org.infinispan.counter.impl.manager.EmbeddedCounterManager.removeCounter(EmbeddedCounterManager.java:128)
		at org.infinispan.counter.impl.manager.EmbeddedCounterManager.undefineCounter(EmbeddedCounterManager.java:137)
Caused by: java.lang.ClassCastException: class org.infinispan.counter.impl.weak.WeakCounterKey cannot be cast to class [B (org.infinispan.counter.impl.weak.WeakCounterKey is in unnamed module of loader 'app'; [B is in module java.base of loader 'bootstrap')
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
	at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
	at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
	at org.infinispan.marshall.exts.ThrowableExternalizer.readGenericThrowable(ThrowableExternalizer.java:282)
	at org.infinispan.marshall.exts.ThrowableExternalizer.readObject(ThrowableExternalizer.java:259)
	at org.infinispan.marshall.exts.ThrowableExternalizer.readObject(ThrowableExternalizer.java:42)
	at org.infinispan.marshall.core.GlobalMarshaller.readWithExternalizer(GlobalMarshaller.java:748)
	at org.infinispan.marshall.core.GlobalMarshaller.readNonNullableObject(GlobalMarshaller.java:729)
	at org.infinispan.marshall.core.GlobalMarshaller.readNullableObject(GlobalMarshaller.java:378)
	at org.infinispan.marshall.core.BytesObjectInput.readObject(BytesObjectInput.java:32)
	at org.infinispan.remoting.responses.ExceptionResponse$Externalizer.readObject(ExceptionResponse.java:49)
	at org.infinispan.remoting.responses.ExceptionResponse$Externalizer.readObject(ExceptionResponse.java:41)
	at org.infinispan.marshall.core.GlobalMarshaller.readWithExternalizer(GlobalMarshaller.java:748)
	at org.infinispan.marshall.core.GlobalMarshaller.readNonNullableObject(GlobalMarshaller.java:729)
	at org.infinispan.marshall.core.GlobalMarshaller.readNullableObject(GlobalMarshaller.java:378)
	at org.infinispan.marshall.core.GlobalMarshaller.objectFromObjectInput(GlobalMarshaller.java:212)
	at org.infinispan.marshall.core.GlobalMarshaller.objectFromByteBuffer(GlobalMarshaller.java:241)
	at org.infinispan.remoting.transport.jgroups.JGroupsTransport.processResponse(JGroupsTransport.java:1385)
	... 21 more

@gustavocoding
Copy link
Author

gustavocoding commented Sep 11, 2019

@danberindei This error happens on master as well. I was able to reproduce it in the CI machine manually. Feel free to merge this PR I'll investigate the issue as part of https://issues.jboss.org/browse/ISPN-10588

@danberindei danberindei merged commit 282262a into infinispan:master Sep 11, 2019
@danberindei
Copy link
Member

Integrated, thanks Gustavo!

@gustavocoding gustavocoding deleted the ISPN-10460_remove_nested_serializer branch September 11, 2019 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants