sajjad-moradi commented on PR #9244: URL: https://github.com/apache/pinot/pull/9244#issuecomment-1224918965
> I feel most folks set their SLAs based on time freshness, not "number of events". So even if we knew we were 10,000 events behind, that might be fine if you have 1,000 events per second but not fine if you have 10 events per seconds. The purpose of this status checker is to consume as many events as possible before enabling query execution. We shouldn't care if the messages are fresh or not. For example, let's say the freshness threshold is 15s and assume there's a huge spike in the topic in last 15s and there are thousands of events available in the topic. The ingestion time for these events is less than the threshold so the status checker announces the consumption has caught up while there are a lot of messages available in the topic to be consumed. > > Anyways, the current PR has duplicate code between OffsetBasedStatusChecker and FreshnessBasedStatusChecker. I think by making a small change to the OffsetBasedStatusChecker, we can achieve the goal of this PR. We can pass in a config for freshness threshold to OffsetBasedStatusChecker and where we get the stream latest offset at startup, if the config is provided, we can get stream latest offset at the current moment and check the freshness then. > > Fair feedback. I opted to use a parent class to keep the functionality separate rather than try to merge them. I feel like this is a better approach at the moment to keep things more easily testable and singularly responsible. > I agree it's easier to develop and test this way, but in the long run duplicated code has a high maintenance cost. Let's iron out the issues and consolidate the two classes into one. -- 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. To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org For additional commands, e-mail: commits-h...@pinot.apache.org
