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

[RHPAM-1942] Fix wrong database configuration in rhpam73-authoring-ha… #238

Merged
merged 1 commit into from Feb 27, 2019

Conversation

sutaakar
Copy link
Contributor

….yaml

Added missing TIMER_SERVICE_DATA_STORE env variable and updated dialect to org.hibernate.dialect.MySQL5Dialect.

Signed-off-by: Karel Suta ksuta@redhat.com

Thanks for submitting your Pull Request!

Please make sure your PR meets the following requirements:

  • Pull Request title is properly formatted: [RHPAM-XYZ] Subject
  • Pull Request contains link to the JIRA issue
  • Pull Request contains description of the issue
  • Pull Request does not include fixes for issues other than the main ticket
  • Attached commits represent units of work and are properly formatted
  • You have read and agreed to the Developer Certificate of Origin (DCO) (see CONTRIBUTING.md)
  • Every commit contains Signed-off-by: Your Name <yourname@example.com> - use git commit -s

@sutaakar
Copy link
Contributor Author

@errantepiphany Please take a look.

- name: KIE_SERVER_PERSISTENCE_DIALECT
value: "org.hibernate.dialect.MySQLDialect"
value: "org.hibernate.dialect.MySQL5Dialect"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sutaakar Ha, I found this error myself when creating the datagrid ha authoring templates. ::) Should we be even more specific, and replace with MySQL57Dialect? Or do you feel MySQL5Dialect is sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that is the question, on premise we currently test with MySQL5InnoDBDialect, however I think we can use MySQL57Dialect as it is the most specific one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if we should parameterize the dialect, but default it to MySQL57Dialect to best align with the default that's already there for MYSQL_IMAGE_STREAM_TAG (which is 5.7). But this way, people can change to MySQL5InnoDBDialect if they want. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me.
I guess this should be reflected on all templates using database, right?

Copy link
Member

Choose a reason for hiding this comment

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

I do agree we should use 5.7 as it is the default as it match the mysql version we use on OpenShift.
The dialect already is configurable, do you mean set something like MySQL${VERSION}InnoDBDialect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using version placeholder isn't ideal as there is no specific dialect for every database version. For MySQL there is MySQL5Dialect, MySQL55Dialect and MySQL57Dialect.

@@ -1189,8 +1189,10 @@ objects:
value: "${APPLICATION_NAME}-mysql"
- name: RHPAM_SERVICE_PORT
value: "3306"
- name: TIMER_SERVICE_DATA_STORE
Copy link
Member

Choose a reason for hiding this comment

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

You don't need to set it, it is automatically configured by the EJB timer scripts.
https://github.com/jboss-container-images/jboss-kie-modules/blob/master/jboss-kie-kieserver/added/launch/jboss-kie-kieserver.sh#L88-L100

The reason we set it on the script is to make sure that the datasource used to configure the datastore will be the ejb timer datasource.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so the TIMER_SERVICE_DATA_STORE can be removed from all our templates then?

Copy link
Member

Choose a reason for hiding this comment

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

yes, it will be overridden anyways.

Copy link
Member

@spolti spolti left a comment

Choose a reason for hiding this comment

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

comment above.

@sutaakar
Copy link
Contributor Author

PR updated, all dialects now have specific template property.

@errantepiphany
Copy link
Collaborator

@spolti , @sutaakar : I'm going to process this PR, but then submit another removing TIMER_SERVICE_DATA_STORE from all the templates.

@errantepiphany errantepiphany merged commit 90ed676 into jboss-container-images:master Feb 27, 2019
@errantepiphany
Copy link
Collaborator

Follow-up PR is here: #239

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants