hi < UDSRQR2GP> just following up on this pull request <http Apache Pinot #general

hi <@UDSRQR2GP> just following up on this pull req...

James Shao

10/23/2019, 6:53 PM

hi @User just following up on this pull request (https://github.com/apache/incubator-pinot/pull/3849) Ting submitted back in Apr. Would you mind share your main concern/blocking issue over this diff? if there wasn't any major concern can we merge this diff in soon as it is bug for anyone relies on external deep storage?

Ting Chen

10/23/2019, 11:16 PM

Subbu and I talked in another thread about this issue. This bug should not affect Low Level Consumer if we let the server directly downloads it from deep storage. But both of us agreed the bug is still an issue for Pinot controller using deep storage. We need a fix this download REST API.

Subbu Subramaniam

10/23/2019, 11:37 PM

I will get back on this in the next couple of days. In general, my view is that the server should download from the external storage if the segment is there. We should change the server to commit to the external storage, and there is a first step PR towards that. https://github.com/apache/incubator-pinot/pull/4713

Subbu Subramaniam

10/23/2019, 11:37 PM

If we extend this to inject a segment committer, then you can write your own segment committer that commits a segment to an external storage directly.

Subbu Subramaniam

10/23/2019, 11:38 PM

The only concern I have with the PR is that every segment download in the local storage case will take one additional disk copy, something I would like to avoid.

Subbu Subramaniam

10/23/2019, 11:38 PM

@Neha Pawar any other ideas?

James Shao

10/23/2019, 11:42 PM

for the concerns over the extra storage for local storage, I think ting has explained in the pull requests the files are supposed to be temporary and short-lived. At most there will N files on disk where N is the number of concurrent download requests. If we need to further clarify about that part or harden that part of logics we could definitely work on that.

James Shao

10/23/2019, 11:43 PM

my thinking is if there is no significant negative impact (assuming extra disk storage is not an issue), should we prioritize the bug fix over optimal solutions?

James Shao

10/23/2019, 11:45 PM

I have been thinking about the recent article I read about google code review standard: https://google.github.io/eng-practices/review/reviewer/standard.html and I wonder if we should move closer to that. Would love to hear your thoughts on that

Neha Pawar

10/24/2019, 12:36 AM

have you tried setting the right URI in the segment metadata ? Subbu and I were just discussing, and this line seems to be the culprit? https://github.com/apache/incubator-pinot/blob/master/pinot-controller/src/main/java/org/apache/pinot/controller/helix/core/realtime/PinotLLCRealtimeSegmentManager.java#L483 If this is fixed, everything else is wired already. The download will use the

PinotFSSegmentFetcher

Ting Chen

10/24/2019, 5:20 PM

Thanks Neha. I think it is the right place to change and fix LLC download. We can give it a test. I think PR #3849 is still needed to fix the public API for segment download from controller. Since the extra disk copy for local FS is the only concern, I propose to add an interface method to PinotFS isLocalFS(). For localFS, we can skip to file copy step. Let me know why you think. @Neha Pawar @Subbu Subramaniam

Subbu Subramaniam

10/24/2019, 5:24 PM

Not sure why the download fix is needed if the URI is fixed.

Subbu Subramaniam

10/24/2019, 5:24 PM

Ah, ok if someone is downloading using curl?

Subbu Subramaniam

10/24/2019, 5:26 PM

Better than adding isLocal, can we add a method to return a File handle of the segment? The localFs can just translate URI to a path and return it, but the other FS can copy to local and return the temp. Not sure how to remove the temp file, though....

Ting Chen

10/24/2019, 5:26 PM

yes. PinotSegmentUploadRestletResource is a public controller API.

Ting Chen

10/24/2019, 5:36 PM

The current PR already gets the handle of the segment file as a URI: final URI segmentFileURI = ControllerConf.getUriFromPath(StringUtil.join("/", provider.getBaseDataDirURI().toString(), tableName, URLEncoder.encode(segmentName, URL_ENCODING_SCHEME)));

Ting Chen

10/24/2019, 5:37 PM

We can use it to test if it is local file or not and then apply different return strategy. Right?

Subbu Subramaniam

10/25/2019, 4:25 PM

we can wait on this PR, the other one is more important (fixing the download URL). We dont' have any use case of public download of realtime segments. Do you?

Ting Chen

10/25/2019, 5:53 PM

@Subbu Subramaniam At this time, we are running on an internal fork for Pinot with patches I filed as in various PRs. My main focus now is to resolve these patches one way or another in the Apache master so that we do not need to carry them in our next release later this quarter or early next year.

Ting Chen

10/25/2019, 5:56 PM

So far, these are the PRs I want to merge in master (we talked about them in our last meeting):

Ting Chen

10/25/2019, 5:58 PM

1. Download URL fix as @Neha Pawar suggested. This actually requires some testing as well.

Ting Chen

10/25/2019, 5:58 PM

2. https://github.com/apache/incubator-pinot/pull/4618

Ting Chen

10/25/2019, 6:01 PM

3. The controller download segment API fix in https://github.com/apache/incubator-pinot/pull/3849 because we actually use them in purpose like offline segment download or debugging.

Ting Chen

10/25/2019, 6:03 PM

So in summary, we want to see resolution of all the above PRs before our next release.

Ting Chen

10/25/2019, 6:15 PM

To your earlier question, " We dont' have any use case of public download of realtime segments. Do you?" I did find a few use cases on our end to use this API beside LLC. One is our offline segment download. While you can argue that we can migrate that as well, the fact this API is available on controller and now already used in various place makes fixing it necessary.

Subbu Subramaniam

10/25/2019, 6:18 PM

I approved your PR on offset not found being treated as a transient error.

Subbu Subramaniam

10/25/2019, 6:24 PM

Are you expecting this in the next open source release? We will be cutting then next open source release from f8c2cf3d8 plus one hotfix. as in branch https://github.com/apache/incubator-pinot/tree/selection_comparator_hotfix

Ting Chen

10/25/2019, 6:26 PM

Thanks for the review. For us, if that is in the next release, it would be great.

Ting Chen

10/25/2019, 6:27 PM

But that is not necessary if hotfix now is too troublesome for you guys.

Subbu Subramaniam

10/25/2019, 6:31 PM

Let me talk to my colleagues here and make a call. We generally do not like to put out something that has NOT been run in our production environment, but these are minor changes.

Ting Chen

10/25/2019, 6:36 PM

thanks. Again no urgency here from our side.

Ting Chen

10/25/2019, 6:36 PM

I have kick off the test now.

Subbu Subramaniam

10/25/2019, 7:52 PM

@Ting Chen I merged it

Ting Chen

10/25/2019, 8:25 PM

great. thank you. Will work on the other PRs mentioned above.

Subbu Subramaniam

10/25/2019, 8:26 PM

@Neha Pawar and I will come to a resolution on the download PR.

Neha Pawar

10/28/2019, 4:31 PM

@Ting Chen I just commented on your PR. Subbu and I discussed that the

isLocal

check seems to be an okay approach for now, amongst all the options

Ting Chen

10/29/2019, 9:48 PM

@Neha Pawar thanks for looking into this. I will modify the PR for this. The current PR is too old and Github seems to have a bug and lost linkage to it. I will file a new PR with the above change.

Neha Pawar

10/29/2019, 10:12 PM

sure, thanks for working on this!

Ting Chen

11/04/2019, 10:58 PM

https://github.com/apache/incubator-pinot/pull/4757

Ting Chen

11/04/2019, 10:58 PM

@Neha Pawar @Subbu Subramaniam Please take a look of this issue when free.

👍 1

Open in Slack

Previous Next