kafka-connect-elasticsearch icon indicating copy to clipboard operation
kafka-connect-elasticsearch copied to clipboard

Change version logging to work with upgraded client

Open ilanjiR opened this issue 4 years ago • 5 comments

Problem

Once we upgraded ES client to 7.9.3, the method we were using for logging ES server versions was changed

Solution

Update to the newer way to getting ES server version

Does this solution apply anywhere else?
  • [ ] yes
  • [ ] no
If yes, where?

Test Strategy

Testing done:
  • [ ] Unit tests
  • [ ] Integration tests
  • [ ] System tests
  • [ ] Manual tests

Release Plan

ilanjiR avatar Jun 30 '21 02:06 ilanjiR

Do we have a unit test for this method? I'd expect it to fail if the underlying implementation changes

writing a unit test for this will be tricky unless we test it against a specific docker container version. I do see some class ElasticsearchContainer that downloads 7.9.3 so you might want to check it out

kpatelatwork avatar Jun 30 '21 02:06 kpatelatwork

Do we have a unit test for this method? I'd expect it to fail if the underlying implementation changes

writing a unit test for this will be tricky unless we test it against a specific docker container version.

I think we need at least to assert what this method returns with whatever mocks we use in this repo. I feel this might have caught the fact that we got a class object instead of a version number. But I might be missing something

kkonstantine avatar Jun 30 '21 02:06 kkonstantine

Do we have a unit test for this method? I'd expect it to fail if the underlying implementation changes

writing a unit test for this will be tricky unless we test it against a specific docker container version.

I think we need at least to assert what this method returns with whatever mocks we use in this repo. I feel this might have caught the fact that we got a class object instead of a version number. But I might be missing something

Got it so what you are saying is extract the line response.getVersion().getNumber() to a method getVersion(MainResonse response). and make it package protected and then mock the call response.getVersion().getNumber() returns a string, That should be easy and much faster/simple than trying to run a container and test. @ilanjiR let me know if you need help.

kpatelatwork avatar Jun 30 '21 16:06 kpatelatwork

Do we have a unit test for this method? I'd expect it to fail if the underlying implementation changes

writing a unit test for this will be tricky unless we test it against a specific docker container version.

I think we need at least to assert what this method returns with whatever mocks we use in this repo. I feel this might have caught the fact that we got a class object instead of a version number. But I might be missing something

Got it so what you are saying is extract the line response.getVersion().getNumber() to a method getVersion(MainResonse response). and make it package protected and then mock the call response.getVersion().getNumber() returns a string, That should be easy and much faster/simple than trying to run a container and test. @ilanjiR let me know if you need help.

Probably test getServerVersion as a whole (which is now private). Which should have the same effect with what you mention.

kkonstantine avatar Jun 30 '21 22:06 kkonstantine

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Ilanji Rajamanickam seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Apr 03 '22 12:04 CLAassistant