n3nash commented on a change in pull request #1274: [HUDI-571] Add 'commits
show archived' command to CLI
URL: https://github.com/apache/incubator-hudi/pull/1274#discussion_r371580262
##########
File path:
hudi-common/src/main/java/org/apache/hudi/common/table/HoodieTableMetaClient.java
##########
@@ -289,11 +288,8 @@ public ConsistencyGuardConfig getConsistencyGuardConfig()
{
*
* @return Active commit timeline
*/
- public synchronized HoodieArchivedTimeline getArchivedTimeline() {
- if (archivedTimeline == null) {
- archivedTimeline = new HoodieArchivedTimeline(this);
- }
- return archivedTimeline;
+ public synchronized HoodieArchivedTimeline getArchivedTimeline(String
startTs, String endTs) {
Review comment:
Isn't the instance variable loaded lazily only when getArchivedTimeline() is
called in which case subsequent cases can use that instance variable. I think
the concern here is that the only method we expose now is
getArchivedTimeline(String startTs, String endTs) and caching the instance
variable from that doesn't make sense as you are saying as well. I'm wondering
if we should keep an instance variable and keep the same method as before
getArchivedTimeline() and then the constructor below is also light. Finally, we
expose a method on the archived timeline to filter(startTs, endTs). This way we
don't have to create multiple objects for each window invocation. WDYT ?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
