sergehuber commented on PR #715: URL: https://github.com/apache/unomi/pull/715#issuecomment-2645919064
Hello @jkevan thank you for your review. I will look at your feedback but to answer some of your questions globally : - I am aware there are some things that don't seem directly related to the OpenSearch work, however ALL of them were necessary to get OpenSearch to work properly, and some where tools that were developped while working on the project to make sure everything worked. It made more sense to include them in the project since separating them would take more time. I do understand the fact that your preference is to have individual changes but on large modifications such as this this actually wastes time that would be better used at moving the project forward. - I will add some JavaDocs as I agree some are missing - About the re-implementation of some classes, these were necessary for the following reasons : stay 100% compatible with the existing implementation. Using other implementations such as the JSR 385 backport would have added possible changes in behavior. A lot of code had to be centralized and therefore new interface had to be created to make sure that the code was properly reusable between the two. - For the missing tests, actually I have been working on a new REST API test framework that includes testing of some of the new methods but I haven't been sharing that yet. The current integration tests are a nightmare to work with, especially on such a large change. - About the changes to features, this was not a change I really wanted to make at this point but was forced to do because there was really no clean way to fix the startup with conditional bundles depending on the persistence implementation. The unomi:start implementation was a mess from the start and was already duplicating a lot of the functionality of Karaf features and ideally should be completely removed to just use features. I actually tried to do that but I ran into BIG problems with the pax-web implementation that wouldn't restart properly. This is why I had to set all the bundles to start=false in the feature and then start them otherwise the system wouldn't start properly. I'm hoping this will be fixed when upgrading to a more recent Karaf but this was the only way I managed to get things to work properly for both Elasticsearch and OpenSearch. - As for the parameter on the unomi:start command this was actually very useful when developing as it made it very easy to start and stop Unomi without starting Karaf to switch between the ElasticSearch and OpenSearch implementations. As it is integrated with the autostart configuration it is also possible to set this up before the start and this has been implemented for the Docker images. - Did you watch the recording of the meeting I did to present the changes? A lot of the questions you had were presented in that meeting, which I specifically organized to explain the changes and give people opportunities to ask questions and give early feedback. You can find the video here : https://www.youtube.com/watch?v=uNbKW29FCnE . I will be organizing more of these as I am actively working on some more important contributions to the project and I have been sharing with the community at the monthly meetings as well. - Last but not least, as I have been working on other major things such as multi-tenancy and other cleanups such as the removal of the clustering code, I am beginning to think that maybe we should move these changes to be part of Unomi V3, which is a branch I have already started working on, and I have also shared my current work in the mailing lists and in multiple meetings. These big changes will slow down quickly however, as I am working on making Unomi much cheaper to deploy and operate. OpenSearch and multi-tenancy are the biggest requirements on this area, along with some cleanups that I would like to do. I am going to communicate some proposals on the subject, but I really need to get these things implemented for my own needs as well as some other users of Unomi. If you are interested in the multi-tenancy, please check my email in the mailing list about it : https://lists.apache.org/list.html?dev@unomi.apache.org -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@unomi.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
