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

Fixed host issue for kafka-interactive-queries #128

Merged
merged 4 commits into from Aug 7, 2018

Conversation

Projects
None yet
6 participants
@nehabhardwaj01
Copy link
Contributor

commented Jun 19, 2018

Currently, any string provided as the APPLICATION_HOST will start the REST Service on localhost (0.0.0.0).

This fix will enhance the functionality by-

  • Validating the HOST and raising the exception for any invalid/ unreachable/ unavailable host.
  • Starts the REST service on the provided host and port (If valid).

Issue link: #127

@ConfluentCLABot

This comment has been minimized.

Copy link

commented Jun 19, 2018

It looks like @nehabhardwaj01 hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

@nehabhardwaj01

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2018

[clabot:check]

@ConfluentCLABot

This comment has been minimized.

Copy link

commented Jun 19, 2018

@confluentinc It looks like @nehabhardwaj01 just signed our Contributor License Agreement. 👍

Always at your service,

clabot

@guozhangwang
Copy link
Member

left a comment

The change lgtm overall except the unstable unit test, would like @dguy @miguno @bbejeck @vvcephei to have a second look at it.


@Test
public void shouldStartRestApiOnAnyValidHost() throws Exception {
// Race condition caveat: This two-step approach of finding a free port but not immediately

This comment has been minimized.

Copy link
@guozhangwang

guozhangwang Jun 20, 2018

Member

Hmm.. this is a it worrisome to me, if we cannot have stable examples repo unit test. Note that we have regular cron jobs to run the unit tests and if anything fails we need to look into it. Having an unstable test due to port binding would reduce the sensitivity of the test. Could we improve on fixing it?

This comment has been minimized.

Copy link
@nehabhardwaj01

nehabhardwaj01 Jun 21, 2018

Author Contributor

Hi @guozhangwang, The original tests in the class also use the same way of binding and the comment is mentioned for them as well.
Although this comment serves as a corner case, still would like to know any suggestions for improvement.

This comment has been minimized.

Copy link
@vvcephei

vvcephei Jun 21, 2018

Member

Ah, yes, I see where we previously had the exact same warning. We do already have some problems with flaky tests, though, and this would make the probability higher of a failure, so it might be a good idea to take the opportunity to make them both more robust.

If we can wrap the search and bind in a retry loop, I think we'd be all good. It's what I've done in the past.

This comment has been minimized.

Copy link
@bbejeck

bbejeck Jun 21, 2018

Contributor

This warning comment is pre-existing.

As for an approach that would help with failures due to the race condition mentioned, the only thing that comes to mind ATM is to do a try/catch around the block of code setting the port and starting the rest proxy server with some reasonable number of retries (2-3?).

This comment has been minimized.

Copy link
@nehabhardwaj01

nehabhardwaj01 Jun 22, 2018

Author Contributor

Agree, Adding a try/catch block will add more robustness to the tests.
Will do the change. 👍

@bbejeck
Copy link
Contributor

left a comment

Overall the change LGTM as well. I've left one minor comment about loading sample songs for the test. I have one suggestion for mitigation the race condition of grabbing a free port before binding, but it's just a suggestion off the top of my head. Since we've been living with this race condition for a while I'm not sure it's enough to hold up merging. It would probably be good to get on more opinion.

\cc @dguy @vvcephei @mjsax @miguno

@@ -67,9 +66,71 @@
private KafkaStreams streams;
private MusicPlaysRestService restProxy;
private int appServerPort;
private static final List<Song> songs = Arrays.asList(new Song(1L,

This comment has been minimized.

Copy link
@bbejeck

bbejeck Jun 21, 2018

Contributor

These sample songs are loaded from a file (thanks @ybyzek ) in io.confluent.examples.streams.interactivequeries.kafkamusic.KafkaMusicExampleDriver. Maybe we could re-use the same approach here?

This comment has been minimized.

Copy link
@nehabhardwaj01

nehabhardwaj01 Jun 22, 2018

Author Contributor

Sure, @bbejeck The approach seems good and applicable. Will apply them. Thanks for pointing it out.


@Test
public void shouldStartRestApiOnAnyValidHost() throws Exception {
// Race condition caveat: This two-step approach of finding a free port but not immediately

This comment has been minimized.

Copy link
@bbejeck

bbejeck Jun 21, 2018

Contributor

This warning comment is pre-existing.

As for an approach that would help with failures due to the race condition mentioned, the only thing that comes to mind ATM is to do a try/catch around the block of code setting the port and starting the rest proxy server with some reasonable number of retries (2-3?).

@@ -236,6 +239,145 @@ public void shouldDemonstrateInteractiveQueries() throws Exception {
assertTrue(keyValueBean.getKey().startsWith("streams"));
assertThat(keyValueBean.getValue(), equalTo(3L));
}

@Test
public void shouldStartRestApiOnAnyValidHost() throws Exception {

This comment has been minimized.

Copy link
@vvcephei

vvcephei Jun 21, 2018

Member

If all this test is verifying is that all the right binding and metadata advertisement happens, I think it would be better to skip the actual IQ logic test (the exiting test already checks it). Similar to your other test that verifies an exception binding to an invalid host, we could just do the setup and do a quick check that everything is hooked up properly.

Then again, I'm not very familiar with this example. Is this suggestion valid?

This comment has been minimized.

Copy link
@nehabhardwaj01

nehabhardwaj01 Jun 22, 2018

Author Contributor

@vvcephei Apart from verifying binding and metadata advertisement, We are testing all those scenarios(IQ logic test) just to ensure that all rest endpoints works as expected with this host as well.
If anyhow, they seem irrelevant, we can surely make the suggested changes.

producerConfig.put(ProducerConfig.VALUE_SERIALIZER_CLASS_CONFIG, StringSerializer.class);
IntegrationTestUtils
.produceValuesSynchronously(WordCountInteractiveQueriesExample.TEXT_LINES_TOPIC, inputValues,
producerConfig);

This comment has been minimized.

Copy link
@vvcephei

vvcephei Jun 21, 2018

Member

I think it would be better to keep this in the method(s) that need(s) it. The reason is that not all tests needs to consume this data, but it will get populated before every test. It's logically fine, but it does cause our tests to run slower.

In fact, I'd recommend leaving this method as you have it, and just removing the @BeforeClass annotation and directly calling this method at the start of the relevant tests.

This comment has been minimized.

Copy link
@nehabhardwaj01

nehabhardwaj01 Jun 22, 2018

Author Contributor

@vvcephei, Maybe there's some confusion, Actually, The @BeforeClass annotation ensures that any computationally expensive setup is executed only once for all those tests that share this setup.
However, @Before will run before each test case.

Reference: http://junit.sourceforge.net/javadoc/org/junit/BeforeClass.html

This comment has been minimized.

Copy link
@vvcephei

vvcephei Jun 22, 2018

Member

Ah, of course. I misread the code. My apologies.


@BeforeClass
public static void createTopics() {
public static void createTopicsAndProduceDataToInputTopics() {

This comment has been minimized.

Copy link
@vvcephei

vvcephei Jun 21, 2018

Member

Please apply my feedback from the WordCountInteractiveQueriesExampleTest.


@After
public void shutdown() throws Exception {
restProxy.stop();

This comment has been minimized.

Copy link
@vvcephei

vvcephei Jun 21, 2018

Member

should we catch and log an exception here, so we'll still try to close streams if restProxy.stop fails?

This comment has been minimized.

Copy link
@nehabhardwaj01

nehabhardwaj01 Jun 22, 2018

Author Contributor

Sure, @vvcephei It'll be a better approach. Will apply it.

}

@Test
public void shouldDemonstrateInteractiveQueriesOnAnyValidHost() throws Exception {

This comment has been minimized.

Copy link
@vvcephei

vvcephei Jun 21, 2018

Member

Similar to my feedback from the WordCountInteractiveQueriesExampleTest, do we need the full validation of IQ here, or is it sufficient to just verify the right host got bound?

@vvcephei

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

Hey @nehabhardwaj01 ,

I left a few comments/suggestions/nitpicks, but it looks good overall.

Thanks for the PR!
-John

Neha Bhardwaj
1. Added Songs CSV for KafkaMusicExampleTest.
2. Added Exception Handling to Prevent Port Binding Race Condition.
3. Added a Test Case to elaborate the log and exception thrown for BindException.
@nehabhardwaj01

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2018

Hi @vvcephei,

Thanks for reviewing the PR, I've answered few of your queries and raised questions for those I might need more clarity.
I've committed the changes and will be great if you can review them once.

@guozhangwang

This comment has been minimized.

Copy link
Member

commented Jun 23, 2018

made another pass and do not have further comments. @vvcephei @bbejeck please feel free to commit / pint merge if it lgty.

@mjsax
Copy link
Member

left a comment

Thanks for the PR @nehabhardwaj01!

Couple of comments.

@@ -202,9 +202,10 @@ public static void main(String[] args) throws Exception {
}


static WordCountInteractiveQueriesRestService startRestProxy(final KafkaStreams streams, final int port)
static WordCountInteractiveQueriesRestService startRestProxy(final KafkaStreams streams, final int port, final
String host)

This comment has been minimized.

Copy link
@mjsax

mjsax Jul 24, 2018

Member

nit: fix formatting

startRestProxy(final KafkaStreams streams,
               final int port,
               final String host)

try {
jettyServer.start();
} catch (java.net.SocketException exception) {

This comment has been minimized.

Copy link
@mjsax

mjsax Jul 24, 2018

Member

nit: add final


jettyServer.start();

ServerConnector connector = new ServerConnector(jettyServer);

This comment has been minimized.

Copy link
@mjsax

mjsax Jul 24, 2018

Member

nit: add final


jettyServer.start();

ServerConnector connector = new ServerConnector(jettyServer);

This comment has been minimized.

Copy link
@mjsax

mjsax Jul 24, 2018

Member

nit: add final


try {
jettyServer.start();
} catch (java.net.SocketException exception) {

This comment has been minimized.

Copy link
@mjsax

mjsax Jul 24, 2018

Member

nit: add final

public void shutdown() throws Exception {
try {
restProxy.stop();
} catch (Exception e) {

This comment has been minimized.

Copy link
@mjsax

mjsax Jul 24, 2018

Member

nit: add final

streams.start();

if (restProxy != null) {

This comment has been minimized.

Copy link
@mjsax

mjsax Jul 24, 2018

Member

as above


@Test
public void shouldDemonstrateInteractiveQueriesOnAnyValidHost() throws Exception {
final String host = "127.10.10.10";

This comment has been minimized.

Copy link
@mjsax

mjsax Jul 24, 2018

Member

fix indention

createStreams(host);
streams.start();

if (restProxy != null) {

This comment has been minimized.

Copy link
@mjsax

mjsax Jul 24, 2018

Member

as above

@@ -299,7 +329,7 @@ private void verifyChart(final String url,
assertThat(chart, is(expectedChart));
}

private void sendPlayEvents(final int count, final Song song,
private static void sendPlayEvents(final int count, final Song song,
final KafkaProducer<String, PlayEvent> producer) {

This comment has been minimized.

Copy link
@mjsax

mjsax Jul 24, 2018

Member

Nit: fix indention and move final Song song to it's own line

Neha Bhardwaj
1. Fixed foramtting and indentation.
2. Added fail() to achieve negative tests.
@nehabhardwaj01

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2018

Hi @mjsax,
Thanks for reviewing the PR, I've added the suggested changes. Will be great if you can review them once.

@mjsax
Copy link
Member

left a comment

Thanks for update! Some formatting nits. Otherwise LGTM. Will merge after addressed.

Thanks a lot for the PR!

@@ -244,6 +245,8 @@ public void shouldDemonstrateInteractiveQueries() throws Exception {
final KeyValueBean keyValueBean = windowedResult.get(0);
assertTrue(keyValueBean.getKey().startsWith("streams"));
assertThat(keyValueBean.getValue(), equalTo(3L));
} else {
fail("Should fail demonstrating InteractiveQueries as the Rest Service failed to start.");

This comment has been minimized.

Copy link
@mjsax

mjsax Aug 6, 2018

Member

nit: fix indention -- should be 2 spaces

@@ -377,6 +380,8 @@ public void shouldStartRestApiOnAnyValidHost() throws Exception {
final KeyValueBean keyValueBean = windowedResult.get(0);
assertTrue(keyValueBean.getKey().startsWith("streams"));
assertThat(keyValueBean.getValue(), equalTo(3L));
} else {
fail("Should fail demonstrating InteractiveQueries on any valid host as the Rest Service failed to start.");

This comment has been minimized.

Copy link
@mjsax

mjsax Aug 6, 2018

Member

nit: fix indention -- should be 2 spaces

@@ -152,20 +153,19 @@ public static void createTopicsAndProduceDataToInputTopics() throws Exception {

private void createStreams(final String host) throws Exception {
appServerPort = randomFreeLocalPort();
streams =
KafkaMusicExample.createChartsStreams(CLUSTER.bootstrapServers(),
streams = KafkaMusicExample.createChartsStreams(CLUSTER.bootstrapServers(),
CLUSTER.schemaRegistryUrl(),

This comment has been minimized.

Copy link
@mjsax

mjsax Aug 6, 2018

Member

nit: fix indention -- parameters should align (also lines below)

Neha Bhardwaj
@nehabhardwaj01

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2018

Hi @mjsax, I've checked in the latest code. Can you please verify the addressed changes.

@mjsax mjsax merged commit 73fd032 into confluentinc:4.1.0-post Aug 7, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details
@mjsax

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

Thanks a lot for the PR @nehabhardwaj01!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.