search

RedHat-Middleware-Workshops / dg8-workshop

Source for the Red Hat Data Grid 8.0 workshop content repository
15 stars 22 forks source link

Externalizing session update #42

Closed edeandrea closed 4 years ago

edeandrea commented 4 years ago

Coincides with RedHat-Middleware-Workshops/dg8-workshop-labs#1

danieloh30 commented 4 years ago

@edeandrea thanks for your contribution. In order to understand your PR clearly before merging, can you take a look at the following inquires?

1) You need to remove the space here: *Red Hat the embeddedCache 2) It was supposed not to use too much backtick so do you have a specific reason to replace bold or italic font with backsticks? 3) org.infinispan.configuration.cache.Configuration class is already imported in the EmbeddedCacheService class and do you have a specific reason why you replaced with final org.infinispan.configuration.cache.Configuration ispnConfig = new ConfigurationBuilder()? 4) Do you have a specific reason why you removed "TODO: Add SessionController class her" part?

edeandrea commented 4 years ago

Hi @danieloh30 see answers. Also note that this PR coincides with https://github.com/RedHat-Middleware-Workshops/dg8-workshop-labs/pull/1 so please take a look at that as well as it may help answer some of these questions.

  1. I will take care of this
  2. It was my thought that whenever we referenced code or class names that we used backtiks
  3. I moved this to a new @Configuration class. Since the Configuration import is now taken for the Spring @Configuration annotation, I needed to be explicit for the org.infinispan.configuration.cache.Configuration class
  4. If you take a look at the code change in https://github.com/RedHat-Middleware-Workshops/dg8-workshop-labs/pull/1 you'll notice the SessionController class I moved to a standalone class that is already there in the codebase vs having the user copy/paste it in. I just think that is cleaner.
danieloh30 commented 4 years ago

2 - I didn't use the backticks for class names all the time so I wonder if you had feedback from customers(devs) about this.

3, #4 - Did you create a PR for that? I don't see any updates from you here: https://github.com/RedHat-Middleware-Workshops/dg8-workshop-labs/tree/ocp-4.4/dg8-spring-session/src/main/java/com/redhat/com/rhdg

edeandrea commented 4 years ago

Wherever possible I replaced all class names with backtiks as I found some were bold, some were backtick'ed, some italicized, and some nothing. I tried best I could to be consistent. I've always found that backticks for anything code related (class names, method names, etc) are preferred. Even here on GitHub most people encapsulate code-related stuff in backtiks.

Yes there is a PR - see https://github.com/RedHat-Middleware-Workshops/dg8-workshop-labs/pull/1

danieloh30 commented 4 years ago

Nope, I don't want to use too many backticks. Can you add guides to create SessionController.java and add codes and import org.infinispan.configuration.cache.Configuration than add it to the code? When you replace all these codes, was the spring session working btw?

edeandrea commented 4 years ago

Yes spring session was working fine with my changes. I thought having the SessionController.java already there in the codebase vs having the user create it would be better since the REST API part of this exercise is not the focus - the configuration of the cache was the focus.

Also the cache configuration (methods with @Bean annotations) belong in @Configuration classes, not @Service classes. That is why I moved them to a config package inside a class called EmbeddedCacheConfig.java with an @Configuration annotation. With the @Configuration annotation you would need to "spell-out" the fully-qualified class name for either the infinispan Configuration class or the spring Configuration class.