-
Notifications
You must be signed in to change notification settings - Fork 1k
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
TEST: adding stream-table join #375
TEST: adding stream-table join #375
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor comments
@@ -17,8 +17,8 @@ | |||
package io.confluent.ksql; | |||
|
|||
import io.confluent.ksql.metastore.MetaStore; | |||
import io.confluent.ksql.util.KafkaTopicClientImpl; | |||
import io.confluent.ksql.util.KsqlConfig; | |||
import io.confluent.ksql.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you probably need to change your import settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err - yep - will do
result.put(key, recordMetadataFuture.get(TEST_RECORD_FUTURE_TIMEOUT_MS, TimeUnit.MILLISECONDS)); | ||
} | ||
producer.close(); | ||
|
||
return result; | ||
} | ||
|
||
private ProducerRecord<String, GenericRow> buildRecord(String topicName, Long timestamp, Map.Entry<String, GenericRow> recordEntry, String key) { | ||
ProducerRecord<String, GenericRow> producerRecord; | ||
if (timestamp == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need this. Can just pass in null for the timestamp
@@ -65,7 +65,7 @@ public void shouldAggregateTumblingWindow() throws Exception { | |||
|
|||
// not really required - but lets mess with ms | |||
Thread.sleep(100); | |||
testHarness.publishTestData(topicName, dataProvider); | |||
testHarness.publishTestData(topicName, dataProvider, null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you get rid of the sleeps in this class now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good spot
@dguy - how does that look? we now control time and the future. |
@@ -64,8 +67,7 @@ public void after() throws Exception { | |||
public void shouldAggregateTumblingWindow() throws Exception { | |||
|
|||
// not really required - but lets mess with ms |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these comments still relevant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor comment otherwise LGTM
retest this please |
#375 - adding missing integration tests