Skip to content

Conversation

@milimetric
Copy link
Contributor

Dear Gobblin maintainers,

This is my first PR, please do point out any mistakes or omissions.

JIRA

GOBBLIN-1409

Description

When consuming from Kafka, we need the kafka timestamp and timestamp type. This change implements the necessary methods.

Tests

  • The existing test was updated to check the new methods.

In our use case, we need the timestamp from the broker, so we also
implement isTimestampLogAppend and a kafka-1 specific method,
getTimestampType, to let us check the TimestampType in all cases.
@milimetric
Copy link
Contributor Author

Apologies for the initial hiccup, I think I straightened it out. cc @ZihanLi58

@codecov-io
Copy link

Codecov Report

Merging #3244 (9f53f71) into master (1cbe3d0) will decrease coverage by 37.37%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #3244       +/-   ##
============================================
- Coverage     46.40%   9.03%   -37.38%     
+ Complexity     9940    1737     -8203     
============================================
  Files          2030    2030               
  Lines         78783   78786        +3     
  Branches       8765    8766        +1     
============================================
- Hits          36561    7117    -29444     
- Misses        38815   70973    +32158     
+ Partials       3407     696     -2711     
Impacted Files Coverage Δ Complexity Δ
...che/gobblin/kafka/client/Kafka1ConsumerClient.java 0.00% <0.00%> (-23.72%) 0.00 <0.00> (-6.00)
...c/main/java/org/apache/gobblin/util/FileUtils.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-4.00%)
...n/java/org/apache/gobblin/fork/CopyableSchema.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-2.00%)
...java/org/apache/gobblin/stream/ControlMessage.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
...va/org/apache/gobblin/dataset/DatasetResolver.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...va/org/apache/gobblin/converter/EmptyIterable.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
...org/apache/gobblin/ack/BasicAckableForTesting.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-3.00%)
...n/java/org/apache/gobblin/salesforce/SfConfig.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
.../org/apache/gobblin/yarn/HelixMessageSubTypes.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-1.00%)
...va/org/apache/gobblin/cluster/SingleHelixTask.java 0.00% <0.00%> (-100.00%) 0.00% <0.00%> (-5.00%)
... and 1074 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cbe3d0...9f53f71. Read the comment docs.

Copy link
Contributor

@ZihanLi58 ZihanLi58 left a comment

Choose a reason for hiding this comment

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

+1 LGTM. @autumnust Can you help merge?

@milimetric
Copy link
Contributor Author

Just a gentle bump here, we're wondering about rough timelines of PRs as we strategize about our deployment of gobblin. Any info is appreciated.

Copy link
Contributor

@shirshanka shirshanka left a comment

Choose a reason for hiding this comment

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

LGTM!

@asfgit asfgit closed this in d799b6a Apr 10, 2021
@shirshanka
Copy link
Contributor

@milimetric : thanks for the contribution and sorry for the delay in merging this in!

@milimetric
Copy link
Contributor Author

milimetric commented Apr 11, 2021 via email

jhsenjaliya pushed a commit to jhsenjaliya/incubator-gobblin that referenced this pull request Jun 7, 2021
In our use case, we need the timestamp from the
broker, so we also
implement isTimestampLogAppend and a kafka-1
specific method,
getTimestampType, to let us check the
TimestampType in all cases.

Closes apache#3244 from milimetric/master
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.

4 participants