Hi Erik, Thanks for taking the time to pull out this ElasticSearch functionality into a new module.
I don't think it's necessary to try to enumerate all possible functionality ... it's far better to just let the use cases drive out the functionality. So I'd rather that the currently working functionality is documented and made available, than try to second guess what new features might be required. Meantime, I've had a look over the code, and have some observations, some trivial and some less so. First, some trivial points: https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/ElasticSearchService.java#L58 - don't think there's any reason for this field to be static ? https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/ElasticSearchService.java#L42 - "cluster.name" ... tend to put a prefix of some sort, eg "isis.services.audit.objects=all" suggest "isis.services.elasticsearch." as the prefix https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/resources/org/isisaddons/module/elasticsearch/search/result/SearchResult.layout.xml for some reason in src/main/resources, whereas other layouts in src/main/java Think that src/main/java is the way to go https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexServiceTest.java this is a test class, should be in src/test/java https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/AbstractIndex.java as a naming convention, I tend to use "Abstract" as a suffix https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L90 settings seem temporarily to be disabled/commented out? https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L31 I tend to mark these as @Programmatic, to keep the size of the metamodel down (avoid https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L123 and https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L133 and https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L157 and https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java#L168 recommend to just throw a new RuntimeException(e) so we can fail fast rather than swallow exceptions https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexerFactory.java this looks like an SPI, so typically put these in an 'spi' subpackage https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResult.java and https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResultsPage.java recommend making these JAXB view model rather than using @ViewModel; more flexible. https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResultsPage.java#L60 can use @Nullable here instead of @Parameter(optionality = Optionality.OPTIONAL) https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/search/result/SearchResult.java#L38 suggest use the ?: ternary operator https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/IndexService.java ? quite a few of the methods in IndexService aren't called, and/or seem incomplete: - addMappings - initialiseSearchEngine - addToBulkRequest https://github.com/erikdehair/isis-module-elasticsearch/blob/master/fixture/src/main/java/org/isisaddons/module/elasticsearch/fixture/dom/ElasticSearchDemoObject.java#L67 the implementation could be simplified to: Bookmark bookmark = bookmarkService2.bookmarkFor(this); //return bookmark.getObjectType() +":"+ bookmark.getIdentifier() return bookmark.toString(); and then perhaps this could be moved into the caller, with Indexabe interface defining getIndexIsd() as optional (return null): - make default String getIndexId() { return null; } ie, and if returns null, then have the caller work out the Id using the bookmark ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ So much for the trivial stuff; I also have some "deeper" questions on the design. As I understand the code, we use Indexable as an interface to determine - via the IndexableSubscriber - which domain objects get added to the ElasticSearch index. To do that, it obtains a corresponding Indexer for the domain object, which then returns an AbstractIndex - basically a DTO which is an extract of the relevant fields. This is serialized to JSON. I see that Index defines a getTenancy() property, and initially wondered whether that made sense, but on second thoughts I think it *does *make sense to include it in the JSON .. applications that don't need app tenancy can just use "/" as a global value. I'd like to pull this concept of tenancy into a separate module within incode-platform, but that can wait til a later date for now. I also see that AbstractIndex has a getType() property. Is this necessary? Why not just use the ObjectSpecId (as per @DomainObject#objectType) of the object being indexed? (One possible answer is if we wanted to bundle indexes of various different domain objects - perhaps different subclasses into a single index, like a "roll-up". But does ElasticSearch do a query across all indexes anyway? If so, then probably no need to have getType() property) Still on AbstractIndex, I wonder if createJson needs to be defined upon it? Why not just have its caller (IndexService) do this serialization? Looking at Indexer : https://github.com/erikdehair/isis-module-elasticsearch/blob/master/dom/src/main/java/org/isisaddons/module/elasticsearch/indexing/Indexer.java first question is, why is this an abstract class rather than an interface ? But a deeper question is... was wondering if we can get rid of Indexer, or at least remove it from the SPI? All it seems to do is to copy out selected properties from the domain object into the corresponding Index (with that Index then just being serialized out to JSON). Perhaps a different design would be extend @Property attribute to indicate which fields are to be included within the index? Then we can obtain the data to be indexed via the metamodel? - or, can do this with a custom annotation (cf summernote and pdfjs), eg @ElasticSearchProperty - with additional facets If it is to be kept as an SPI, then perhaps allow multiple implementations of IndexerFactory to be implemented (with these injected using @Inject List<IndexerFactory>). I have a feeling that maybe some SPI is required for settings too; maybe these concepts can be unified? ~~~~~~~~~ Hope the above is of use. As you might recall from the mini-conference, I've been pulling together all the existing addons into a single "platform" [1]. This is till work in progress, but once I have that assembled and restructured, I intend to put some coding standards/guidelines as to what should be in a framework module - as you rightly point out, the code is written for re-use so can make fewer assumptions about how it is called, and should provide more hooks. For example, one of the things I've tried to do for any modules that define entities - eg security - is to ensure that every property/colleciton/action is annotated with a domain event; this makes it easy for consuming applications to selectively hide or disable functionality. There's also a hierarchy for these domain event classes. Another example might be to use title events rather than title() - I haven't done this consistently but would like to; again it provides a consistent hook for consuming apps. Another point is to ensure that any public API (as used by domain objects) should be in a separate maven module. This avoid polluting the classpath of the domain modules with unnecessary implementation classes (eg the ElasticSearch client). What I would like to do though, if you're happy, is to bring in this ElasticSearch into the incode-platform. How does that sound? Thanks again Dan [1] https://github.com/incodehq/incode-platform On Thu, 13 Jul 2017 at 18:39 Erik de Hair <[email protected]> wrote: > Right now the module can do indexing and a basic query can be executed > to find some different types of demo objects and boost a particular type > of demo object, so it will show up at the top of the result list. > > Still have to simplify some things and write a readme/documentation. The > demo application is a based on the module template so it can be run as > every other module demo application. The only thing is: you need an > installed ElasticSearch cluster/node. > > The question about this elastic search module is: what features should > it offer? > > What i've implemented is: > * Manage connection to a ES cluster > * Indexing of entities marked as Indexable > * A basic search service that offers an action to find some entity based > on the content as defined by the 'Indexer' of that entity. > * A search results page to show the results in descending order of the > search score and to refine your previous search. > * Boosting of some preferred type to give it a higher score. > > Writing a framework module is something different than implementing > one's company's domain model. It's hard to decide how things should be > done and to determine the level of how generic it must be. So far I've > only been trying to let it do what it should do. Didn't spend much time > on the architecture. Reviews/comments very welcome! > > Erik > > > On 06/22/2017 03:13 PM, Erik de Hair wrote: > > Hi all, > > > > As promised, I started working on an ElasticSearch module. It can be > > found at [1]. There's still a lot to do but at least it's running and > > indexing demo objects. I'll keep you updated about the progress. I > > probably have to write a README before you can give it a try. > > > > Erik > > > > [1] https://github.com/erikdehair/isis-module-elasticsearch > >
