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

7612 - fix(ingest): No more JNDI lookup fails during deploy #7613

Merged
merged 1 commit into from Feb 24, 2021

Conversation

poikilotherm
Copy link
Contributor

What this PR does / why we need it:
As we produce the JMS topic and factory for Ingest in
IngestQueueProducer since Dataverse 5.3, error messages
about unsupported missing message-destination appeared.
This commit fixes the underlying cause by using lookup()
instead of name() for the @resource dependency injection.

Which issue(s) this PR closes:

Closes #7612

Special notes for your reviewer:
Needs manual testing of Ingest!

Suggestions on how to test this:
Usual setup. Try real ingest, so the JMS part is actually used. Not sure API tests already cover that.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
None.

Is there a release notes update needed for this change?:
Nope.

Additional documentation:
None.

As we produce the JMS topic and factory for Ingest in
`IngestQueueProducer` since Dataverse 5.3, error messages
about unsupported missing message-destination appeared.
This commit fixes the underlying cause by using lookup()
instead of name() for the @resource dependency injection.

Solves IQSS#7612
@poikilotherm poikilotherm changed the title fix(ingest): No more JNDI lookup fails during deploy 7612 - fix(ingest): No more JNDI lookup fails during deploy Feb 17, 2021
@pdurbin pdurbin self-assigned this Feb 22, 2021
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

All the tests look fine at https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-7613/4/testReport/ and I'm sure some ingest happens as part of them, so I'm pretty sure we can move this to QA. I didn't run the code to see if the offending line is gone from server.log.

I looked at the eehelp link and poked around a bit in javadoc and stackoverflow. I was hoping for more clear guidance on when "name" vs. "lookup" should be used. If we want, we could always add a line about this in our coding style page in the dev guide.

I noticed that @Resource(name = "mail/notifyMailSession") ("name" instead of "lookup") is still found in MailServiceBean. @poikilotherm do you think we should change this now (in this pull request) or later?

@pdurbin pdurbin assigned poikilotherm and unassigned pdurbin Feb 22, 2021
@djbrooke djbrooke moved this from Review 🦁 to Community Dev 💻❤️ in IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) Feb 22, 2021
@poikilotherm
Copy link
Contributor Author

I looked at the eehelp link and poked around a bit in javadoc and stackoverflow. I was hoping for more clear guidance on when "name" vs. "lookup" should be used. If we want, we could always add a line about this in our coding style page in the dev guide.

I wasn't able to find a good explanation in plain english. It's all a bit vague why we cannot use the name if we provide the resource via annotations.

I noticed that @Resource(name = "mail/notifyMailSession") ("name" instead of "lookup") is still found in MailServiceBean. @poikilotherm do you think we should change this now (in this pull request) or later?

Certainly. May be done as part of #7424

@pdurbin
Copy link
Member

pdurbin commented Feb 23, 2021

@poikilotherm good idea to defer the mail server work until that other issue.

Off to QA.

IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from Community Dev 💻❤️ to QA 🔎✅ Feb 23, 2021
@kcondon kcondon self-assigned this Feb 24, 2021
@kcondon kcondon merged commit b2726b1 into IQSS:develop Feb 24, 2021
IQSS/dataverse (TO BE RETIRED / DELETED in favor of project 34) automation moved this from QA 🔎✅ to Done 🚀 Feb 24, 2021
@djbrooke djbrooke added this to the 5.4 milestone Feb 24, 2021
@poikilotherm poikilotherm deleted the 7612-fix-deploy-jms-err branch December 15, 2021 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
4 participants