KAFKA-6424: QueryableStateIntegrationTest#queryOnRebalance should accept raw text #4549

h314to · 2018-02-08T11:49:55Z

Remove to .toLowerCase in word count stream
Add method to read text from file, and move text to resources file
Minor cleanup: change argument order in assertEquals

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

…ept raw text * Remove to .toLowerCase in word count stream * Add method to read text from file, and move text to resources file * Minor cleanup: change argument order in assertEquals

mjsax · 2018-02-08T17:37:03Z

Build error is related:

Unknown license: /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk9-scala2.12/streams/src/test/resources/data/sampleText.txt

We cannot just a file without license header. Please move back to in-memory. Thanks.

h314to · 2018-02-08T18:05:21Z

Thanks. I missed that on the output. I'm moving it back to in-memory.

…onTest

mjsax · 2018-02-08T19:28:30Z

Call for review @tedyu. I am not sure if this PR addresses the JIRA. As you create the JIRA, you should be able to clarify. Thanks.

tedyu · 2018-02-08T19:48:29Z

My intention was to allow the test to read from an external file.
File has bigger capacity which can test related code at higher scale.

mjsax · 2018-02-08T20:01:04Z

@tedyu Would the file path/name be hardcoded? Or how to pass in the file name? We need the in-memory fall-back as we cannot just add a file without license header.

An alternative would be, to create the file at startup and read it back during the test.

tedyu · 2018-02-08T20:36:31Z

Does data file have to contain license header ?

Hardcoding file path is fine - if the file doesn't exist, we can fall back to the current text.

Thanks

h314to · 2018-02-08T21:01:15Z

If we want to have the text in a file and the file must have a license I could just skip the license while reading, or parse the license lines, adding them to the word count stream, and adjust the test to accommodate this change.

tedyu · 2018-02-08T21:06:33Z

Either way is fine.
Probably skipping the header is less work for you.

Thanks

tedyu · 2018-02-09T17:47:51Z

streams/src/test/resources/data/sampleText.txt

+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+


Can you add a comment here saying not to add more line(s) above this line ?

Thinking about this twice, it might be preferable to not add this file because the parsing logic skipping the header is non-intuitive. We could instead keep the in-memory data for regular test, but add a check if this file exist and if yes, use the file instead of in-memory data. WDYT?

mjsax · 2018-02-09T23:21:59Z

streams/src/test/resources/data/sampleText.txt

+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+


Thinking about this twice, it might be preferable to not add this file because the parsing logic skipping the header is non-intuitive. We could instead keep the in-memory data for regular test, but add a check if this file exist and if yes, use the file instead of in-memory data. WDYT?

mjsax · 2018-02-09T23:26:50Z

streams/src/test/java/org/apache/kafka/streams/integration/QueryableStateIntegrationTest.java

-            "eat sleep rave repeat",
-            "one jolly sailor",
-            "king of the world");
+        inputValues = readInputValues("data/sampleText.txt", 15);


Thinking about this, it might be better to keep in-memory as default for the case the file does not exist and read the file if it exist overwriting the in-memory data. This keep the test itself cleaner and avoid the header skipping logic that is quite unintuitive.

For large files, I am wondering, if reading the whole file into an array is good enough... What file sized did you have in mind @tedyu ?

About 5x to 10x the size of what the array holds.

h314to · 2018-02-12T11:42:44Z

I've done a few things to try to address your comments. I removed the header skipping logic and now default to a list of strings that is enough to run the tests.

I documented the method to make what it is doing clear, and changed the default file name to resources/QueryableStateIntegrationTest/inputValues.txt which I think is a bit more obvious.

mjsax

LGTM.

\cc @tedyu Call for review.

tedyu · 2018-02-12T20:02:21Z

streams/src/test/java/org/apache/kafka/streams/integration/QueryableStateIntegrationTest.java

+                input.add(line);
+            }
+        } catch (Exception e) {
+            input = Arrays.asList(


Add a log saying that internal strings are used since inputValues.txt is absent.

mjsax · 2018-02-14T19:22:33Z

streams/src/test/java/org/apache/kafka/streams/integration/QueryableStateIntegrationTest.java

+     */
+    private List<String> getInputValues() {
+        List<String> input = new ArrayList<>();
+        ClassLoader classLoader = getClass().getClassLoader();


Just some more nits. Can you add final wherever possible: ClassLoader, String filename, BufferedReader, for(final String...), Exception

mjsax · 2018-02-14T19:22:52Z

streams/src/test/java/org/apache/kafka/streams/integration/QueryableStateIntegrationTest.java

+    private List<String> getInputValues() {
+        List<String> input = new ArrayList<>();
+        ClassLoader classLoader = getClass().getClassLoader();
+        String fileName = "QueryableStateIntegrationTest/inputValues.txt";


Use File.separator instead of /

mjsax · 2018-02-14T19:23:03Z

streams/src/test/java/org/apache/kafka/streams/integration/QueryableStateIntegrationTest.java

+                input.add(line);
+            }
+        } catch (Exception e) {
+            log.warn("Unable to read '{}'. Using default inputValues list", "resources/" + fileName);


as above: avoid /

Update to

log.warn("Unable to read '{}{}{}'. Using default inputValues list", "resources", File.seperator, fileName);

Yes, that's certainly safer. Thanks. I also changed it in fileName and added final as requested.

mjsax

Thanks for the update @h314to . Will merge after we get a green build.

mjsax · 2018-02-15T19:11:56Z

Retest this please.

mjsax · 2018-02-15T21:12:12Z

Retest this please.

mjsax · 2018-02-16T20:36:37Z

Merged to trunk

Thanks for the PR @h314to !

h314to · 2018-02-16T20:48:26Z

Great! Thanks for the helpful reviews.

KAFKA-6424: QueryableStateIntegrationTest#queryOnRebalance should acc…

94c922e

…ept raw text * Remove to .toLowerCase in word count stream * Add method to read text from file, and move text to resources file * Minor cleanup: change argument order in assertEquals

mjsax requested review from dguy, mjsax and guozhangwang February 8, 2018 17:34

mjsax added docs streams and removed docs labels Feb 8, 2018

h314to added 2 commits February 8, 2018 18:07

Remove sampleText.txt and move text back into QueryableStateIntegrati…

3e8400d

…onTest

Add uppercase word to inputValues

9da9355

Add method to read file again and move text to resources

350abb8

tedyu reviewed Feb 9, 2018

View reviewed changes

mjsax reviewed Feb 9, 2018

View reviewed changes

Delete input file from resources and default to original test list.

31fa1ca

mjsax approved these changes Feb 12, 2018

View reviewed changes

tedyu approved these changes Feb 12, 2018

View reviewed changes

Log that the default inputValues list is being used

f1cec0c

mjsax reviewed Feb 14, 2018

View reviewed changes

Add final and use File.separator instead of /

06b9dc3

mjsax approved these changes Feb 15, 2018

View reviewed changes

mjsax merged commit 7303f41 into apache:trunk Feb 16, 2018

h314to deleted the fix/KAFKA-6424 branch February 17, 2018 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-6424: QueryableStateIntegrationTest#queryOnRebalance should accept raw text #4549

KAFKA-6424: QueryableStateIntegrationTest#queryOnRebalance should accept raw text #4549

h314to commented Feb 8, 2018 •

edited

Loading

mjsax commented Feb 8, 2018

h314to commented Feb 8, 2018

mjsax commented Feb 8, 2018

tedyu commented Feb 8, 2018

mjsax commented Feb 8, 2018

tedyu commented Feb 8, 2018

h314to commented Feb 8, 2018

tedyu commented Feb 8, 2018

tedyu Feb 9, 2018

mjsax Feb 9, 2018

mjsax Feb 9, 2018

mjsax Feb 9, 2018

tedyu Feb 9, 2018

h314to commented Feb 12, 2018 •

edited

Loading

mjsax left a comment

tedyu Feb 12, 2018

mjsax Feb 14, 2018

mjsax Feb 14, 2018

mjsax Feb 14, 2018

h314to Feb 15, 2018

mjsax left a comment

mjsax commented Feb 15, 2018

mjsax commented Feb 15, 2018

mjsax commented Feb 16, 2018

h314to commented Feb 16, 2018

KAFKA-6424: QueryableStateIntegrationTest#queryOnRebalance should accept raw text #4549

KAFKA-6424: QueryableStateIntegrationTest#queryOnRebalance should accept raw text #4549

Conversation

h314to commented Feb 8, 2018 • edited Loading

Committer Checklist (excluded from commit message)

mjsax commented Feb 8, 2018

h314to commented Feb 8, 2018

mjsax commented Feb 8, 2018

tedyu commented Feb 8, 2018

mjsax commented Feb 8, 2018

tedyu commented Feb 8, 2018

h314to commented Feb 8, 2018

tedyu commented Feb 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

h314to commented Feb 12, 2018 • edited Loading

mjsax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjsax left a comment

Choose a reason for hiding this comment

mjsax commented Feb 15, 2018

mjsax commented Feb 15, 2018

mjsax commented Feb 16, 2018

h314to commented Feb 16, 2018

h314to commented Feb 8, 2018 •

edited

Loading

h314to commented Feb 12, 2018 •

edited

Loading