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

Read project and collection from fabric metadata section #13

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Read project and collection from fabric metadata section #13

wants to merge 7 commits into from

Conversation

RameshByndoor
Copy link
Contributor

with this we will be having full support for fabric document structure.
project and collection will be read from "metadata" section of fabric.
& WHITELIST will be project+collection to avoid collection name clashes.

@buremba
Copy link
Member

buremba commented Feb 4, 2018

Thanks for the commit. It looks like some tests are failing because of this error:

2018-02-04 18:13:15 SEVERE Unable to parse collection from message in Kafka topic.
java.lang.NullPointerException: schemaName is null
	at com.facebook.presto.spi.SchemaUtil.checkNotEmpty(SchemaUtil.java:25)
	at com.facebook.presto.spi.SchemaTableName.<init>(SchemaTableName.java:32)
	at io.rakam.presto.deserialization.json.FabricJsonDeserializer.getTable(FabricJsonDeserializer.java:119)
	at io.rakam.presto.kafka.KafkaJsonMessageTransformer.extractCollection(KafkaJsonMessageTransformer.java:33)
	at io.rakam.presto.kafka.KafkaJsonMessageTransformer.extractCollection(KafkaJsonMessageTransformer.java:19)
	at io.rakam.presto.deserialization.json.JsonMessageEventTransformer.createPageTable(JsonMessageEventTransformer.java:44)
	at io.rakam.presto.TestKafkaJsonDeserializer.testNewCollection(TestKafkaJsonDeserializer.java:101)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:85)
	at org.testng.internal.Invoker.invokeMethod(Invoker.java:639)
	at org.testng.internal.Invoker.invokeTestMethod(Invoker.java:821)
	at org.testng.internal.Invoker.invokeTestMethods(Invoker.java:1131)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:124)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:108)
	at org.testng.TestRunner.privateRun(TestRunner.java:773)
	at org.testng.TestRunner.run(TestRunner.java:623)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:357)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:352)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:310)
	at org.testng.SuiteRunner.run(SuiteRunner.java:259)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:86)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1185)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1110)
	at org.testng.TestNG.run(TestNG.java:1018)
	at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:135)
	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeMulti(TestNGDirectoryTestSuite.java:193)
	at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:94)
	at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:146)
	at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:373)
	at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:334)
	at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:119)
	at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:407)

Also I added a review comment to the PR. Once the tests are fixed and the issue in the PR comment is solved, I will merge the PR.

@RameshByndoor
Copy link
Contributor Author

Thanks @buremba, Fixed all testcases. Please check.

}
catch (Throwable e) {
log.error(e, "Error while parsing record");
continue;
}

if (!whiteListCollections.apply(collection)) {
if (!whiteListCollections.apply(project + collection)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

project + collection is expensive since we're calling this for each record and String instances are immutable. We can add project field to RecordData and also whiteListCollections can be Predicate<RecordData>, that was is more reliable and efficient IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What to do with recorddata.date? that will never match.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use recorddata.date in order to determine whether the record is historical or realtime. Please see dayOfRecord variable in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant If whiteListCollectionsis Predicate<RecordData> then whiteListCollections.apply(curRecordData) would give false because of curRecordData.date.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whiteListCollections should not actually do anything with curRecordData.date, it can return true or false based on curRecordDate.collection and curRecordDate.project. Inside the predicate, it can check whether they are whilelisted or not.

@buremba
Copy link
Member

buremba commented Feb 6, 2018

@RameshByndoor looks cool, thanks! Could you please take a look my comment review?

@satendra-sahu satendra-sahu force-pushed the master branch 3 times, most recently from 92d06ee to 3935bbf Compare February 27, 2018 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants