Bq sink connector using depot repo. #168
Conversation
| @@ -1,8 +1,8 @@ | |||
| package io.odpf.firehose.message; | |||
|
|
|||
|
|
|||
| import io.odpf.firehose.error.ErrorInfo; | |||
| import io.odpf.firehose.error.ErrorType; | |||
| import io.odpf.depot.error.ErrorInfo; | |||
There was a problem hiding this comment.
as I understand the scope of this PR is to introduce depot library for only bigquery sink first, but here we are introducing errortype from depot in message.java which is used across all sink. Which basically means we are affecting all the sinks in firehose.
There was a problem hiding this comment.
This Error info is sink related. Common changes are a pre-requisite.
| * <p> | ||
| * Handle logging and metric capturing. | ||
| */ | ||
| public class FirehoseInstrumentation extends Instrumentation { |
There was a problem hiding this comment.
FirehoseInstrumentation extends Instrumentation which is in depot library,
this change is propagated to all sinks instead of just bigquery sink in this PR
There was a problem hiding this comment.
Yes. It is intentional.
| this.config = addAdditionalConfigs(System.getenv()); | ||
| } | ||
|
|
||
| private Map<String, String> addAdditionalConfigs(Map<String, String> env) { |
There was a problem hiding this comment.
suggestion to rename function to addAdditionalConfigs to addAdditionalDepotSinkConnectorConfigs
| case BIGQUERY: | ||
| bigQuerySinkFactory = new BigQuerySinkFactory(config, statsDReporter); | ||
| config.put("SINK_BIGQUERY_METADATA_COLUMNS_TYPES", |
There was a problem hiding this comment.
we are adding couple configs here and in addAdditionalConfigs function , lets add test for for this. might have to use powermock to validate constructor called
There was a problem hiding this comment.
I will keep additional functions out of factory. So that they will be testable separately.
|
|
||
| Assert.assertTrue(Arrays.equals((byte[]) expectedMessage2.getLogKey(), (byte[]) actualMessage2.getLogKey())); | ||
| Assert.assertTrue(Arrays.equals((byte[]) expectedMessage2.getLogMessage(), (byte[]) actualMessage2.getLogMessage())); | ||
| Assert.assertEquals(expectedMessage2.getMetadata(), actualMessage2.getMetadata()); |
There was a problem hiding this comment.
can we stick to common convetion in the project of using static import from org.junit.Assert and org.mockito.Mockito.
Given we frequently access to static members from these two classes. By using static import, the syntax is more concise and my test is more readable.
There was a problem hiding this comment.
I am not sure we are following any standard. I don't really like static import. Using full function name is more readable for me.
lavkesh
force-pushed
the
bq-sink-connector
branch
from
e6f9e83
to
611daaf
Compare
|
new changes pushed after the review LGTM |
* feat: adding odpf/depot depedencies * feat: add github maven repo for odpf/depot * feat: add github auth * chore: depenedencies * chore: version bump * chore: version bump * refactor: move common code to seperate class
* feat: adding odpf/depot depedencies * feat: add github maven repo for odpf/depot * feat: add github auth * chore: depenedencies * chore: version bump * chore: version bump * refactor: move common code to seperate class
* feat: adding odpf/depot depedencies * feat: add github maven repo for odpf/depot * feat: add github auth * chore: depenedencies * chore: version bump * chore: version bump * refactor: move common code to seperate class
BQ sink using odpf/depot