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

Implements isOpen() / close() methods on RemoteCollection #2881

Merged
merged 1 commit into from Dec 19, 2019

Conversation

@reinhapa
Copy link
Contributor

reinhapa commented Jul 15, 2019

Description:

Implements the Collection.isOpen() method behaviour and also moved duplicated XmlRpcClient acces into a central method on the RemoteCollection eliminating repetitive implementations all over multiple places reducing the total amount of code.

Type of tests:

Adds a new JUnit tests for the new implemented parts as the enhanced parts from Leasable

@dizzzz dizzzz requested review from dizzzz and adamretter Jul 16, 2019
@dizzzz

This comment has been minimized.

Copy link
Member

dizzzz commented Jul 16, 2019

@duncdrum there is a docker issue (java11): any idea?

image

@dizzzz dizzzz requested a review from duncdrum Jul 16, 2019
@dizzzz
dizzzz approved these changes Jul 16, 2019
Copy link
Member

dizzzz left a comment

Nice improvement and simplification of the code. thnx!

@dizzzz

This comment has been minimized.

Copy link
Member

dizzzz commented Jul 16, 2019

@adamretter do you want to have this in 5.x as well?

@dizzzz
dizzzz approved these changes Jul 17, 2019
@dizzzz

This comment has been minimized.

Copy link
Member

dizzzz commented Jul 17, 2019

please could you rebase ? the docker issues are biting the PR :-(

@reinhapa reinhapa force-pushed the reinhapa:remotecollection_open branch from 5b43b9f to 0b0a062 Jul 17, 2019
Copy link
Member

adamretter left a comment

If I understand correctly, now only RemoteCollection#execute has the XML-RPC client. As such I wonder if we need the Leasable stuff at all. Does RemoteCollection "own" the XML-RPC client object?

@adamretter

This comment has been minimized.

Copy link
Member

adamretter commented Jul 18, 2019

Interesting stuff. I don't quite understand all the details yet... so I think it would be useful to have a call to discuss the design of this ;-)

Also, we would need to update all XML-RPC (and XML:DB?) tests in the test suite, as very few of them actually call Collection#close currently :-(

@reinhapa

This comment has been minimized.

Copy link
Contributor Author

reinhapa commented Jul 18, 2019

@adamretter we could have a call to discuss this change any time you like...

@reinhapa reinhapa force-pushed the reinhapa:remotecollection_open branch from 0b0a062 to 23fb4d0 Jul 20, 2019
Copy link
Contributor

duncdrum left a comment

I have no opinion on leasable the CI glitches should be solved. OK from my side @adamretter @reinhapa how about the two of you?

Copy link
Member

dizzzz left a comment

image

@reinhapa reinhapa requested review from adamretter and dizzzz Oct 25, 2019
@adamretter

This comment has been minimized.

Copy link
Member

adamretter commented Oct 25, 2019

@reinhapa sorry for the delay. The scale of this change has put me off a little. I will try and find some time for it ASAP

Copy link
Member

adamretter left a comment

The code itself is fine.

However, my concern that I previously expressed is that we have many classes that use the XML:DB API but don't call close on the Collection or Resource would also need to be addressed to ensure we don't leak resources.

Whilst I agree your approach simplifies the code base. I do think it conflates RemoteCollection with RpcClient. Some of the services don't need a Collection, but we now have to pass a Collection just to get the RpcClient... I don't really like mixing those concerns. Maybe there is a different way that we could reduce code but not mix concerns... could we abstract the Execute into a higher-order-function in a utility class method?

@reinhapa

This comment has been minimized.

Copy link
Contributor Author

reinhapa commented Oct 25, 2019

@adamretter I will check if I can find all the tests that will need to close the collections. Also I found only the following classes where I added the RemoteCollection for the simplification of the remote call so far (maybe I missed an other one):
RemoteDatabaseInstanceManager
RemoteRestoreService
for those I introduced a RemoteCallSite functional interface that specifies the needed method signature and does so hides away the actual needed RPC client implementation. There is only one spot for changing the password, where I would suggest to introduce a "virtual" method name for changing the password in order to remove the need for passing in the client reference, but I think I will do that in a next PR.

@duncdrum duncdrum added this to the eXist-5.1.0 milestone Nov 11, 2019
@duncdrum

This comment has been minimized.

Copy link
Contributor

duncdrum commented Nov 11, 2019

i have the feeling that we'll only know the true extend on missing close calls, if we move this one ahead, to give the wider community a chance to locate problem areas.

@adamretter adamretter modified the milestones: eXist-5.1.0, eXist-5.2.0 Nov 19, 2019
@reinhapa

This comment has been minimized.

Copy link
Contributor Author

reinhapa commented Dec 3, 2019

I will rebase the source in order to tidy up all changes into one commit

@reinhapa reinhapa force-pushed the reinhapa:remotecollection_open branch 3 times, most recently from 0fa83f3 to ad9ece4 Dec 3, 2019
@dizzzz
dizzzz approved these changes Dec 10, 2019
Copy link
Member

dizzzz left a comment

nice cleanup of code....

@dizzzz

This comment has been minimized.

Copy link
Member

dizzzz commented Dec 10, 2019

@adamretter please could you mark your concerns on 'resolved' if you agree with the answers?

@reinhapa reinhapa force-pushed the reinhapa:remotecollection_open branch 2 times, most recently from 77e7aea to d59bf24 Dec 14, 2019
@reinhapa reinhapa force-pushed the reinhapa:remotecollection_open branch from d59bf24 to e00a7ed Dec 17, 2019
@adamretter adamretter merged commit 35176e5 into eXist-db:develop Dec 19, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Travis CI - Pull Request Build Passed
Details
@adamretter

This comment has been minimized.

Copy link
Member

adamretter commented Dec 19, 2019

@reinhapa thanks for your contribution and sorry for the delays.

@reinhapa reinhapa deleted the reinhapa:remotecollection_open branch Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.