Skip to content

Conversation

@bvaradar
Copy link
Contributor

@bvaradar bvaradar commented Jul 2, 2019

No description provided.

@bvaradar bvaradar requested review from n3nash and vinothchandar July 2, 2019 01:20
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already a getCommitsAndCompactionTimeline in HoodieActiveTimeline, do we need this ?

Copy link
Member

Choose a reason for hiding this comment

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

+1

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 need similar method in HoodieTimeline interface. I have renamed the method to getCommitsAndCompactionTimeline so that they will be overridden by HoodieDefaultTimeline and HoodieActiveTImeline.

Copy link
Contributor

@n3nash n3nash left a comment

Choose a reason for hiding this comment

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

left some comments

Copy link
Member

@vinothchandar vinothchandar left a comment

Choose a reason for hiding this comment

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

https://issues.apache.org/jira/browse/HUDI-162 Can you add more context around the issue here

Copy link
Member

Choose a reason for hiding this comment

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

is this needed? seems redundant with getTimeline()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to distinguish active timeline from commitsAndCompactionTimeline

Copy link
Member

Choose a reason for hiding this comment

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

+1

@bvaradar
Copy link
Contributor Author

@n3nash @vinothchandar : Addressed/Replied-to review comments. Unit tests and automated hoodie-demo (manually triggered) ran fine. As discussed earlier, will go ahead merging this change.

@cdmikechen : I addressed the exception " java.lang.IllegalArgumentException: Can not create a Path from an empty string" as part of this PR. The pattern was found in few other places and I felt it is best handled together.

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.

3 participants