refactor segment loading logic in BaseTableDataManager to decouple it with local segment directory #7969

klsince · 2021-12-30T07:10:29Z

This PR tries to refactor the segment loading logic in BaseTableDataManager to rely on SegmentDirectory interface instead of always assuming a concrete local segment directory, to make the logic more extensible, allowing the processed segment to be kept on local disk or remote fs transparently.

For that, we have added a copyTo() method in SegmentDirectory abstract class, and use it to decouple the segment loading logic from where the processed segment is kept physically. Because the segment reloading logic is already conducted on the copy of index directory in order to handle reloading failure, the new copyTo() method fits into the existing logic flow well.

Description

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed

Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

Documentation

codecov-commenter · 2021-12-30T07:46:01Z

Codecov Report

Merging #7969 (ad6d167) into master (823aa07) will decrease coverage by 0.04%.
The diff coverage is 75.00%.

@@             Coverage Diff              @@
##             master    #7969      +/-   ##
============================================
- Coverage     71.35%   71.31%   -0.05%     
- Complexity     4218     4221       +3     
============================================
  Files          1595     1596       +1     
  Lines         82748    82874     +126     
  Branches      12348    12365      +17     
============================================
+ Hits          59046    59102      +56     
- Misses        19715    19766      +51     
- Partials       3987     4006      +19

Flag	Coverage Δ
integration1	`28.97% <27.90%> (-0.10%)`	⬇️
integration2	`27.63% <27.32%> (+0.07%)`	⬆️
unittests1	`68.13% <78.06%> (-0.05%)`	⬇️
unittests2	`14.33% <0.00%> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ent/index/column/PhysicalColumnIndexContainer.java	`90.83% <0.00%> (-0.84%)`	⬇️
...ache/pinot/segment/spi/store/SegmentDirectory.java	`55.55% <0.00%> (-6.95%)`	⬇️
...egment/spi/index/metadata/SegmentMetadataImpl.java	`74.28% <28.57%> (-0.29%)`	⬇️
...server/starter/helix/HelixInstanceDataManager.java	`79.44% <47.05%> (-2.64%)`	⬇️
...indexsegment/immutable/ImmutableSegmentLoader.java	`84.15% <73.52%> (-7.51%)`	⬇️
.../pinot/core/data/manager/BaseTableDataManager.java	`87.44% <80.24%> (+2.67%)`	⬆️
...ocal/segment/index/loader/SegmentPreProcessor.java	`80.00% <83.33%> (+0.21%)`	⬆️
...nt/local/loader/DefaultSegmentDirectoryLoader.java	`100.00% <100.00%> (ø)`
.../columnminmaxvalue/ColumnMinMaxValueGenerator.java	`73.68% <100.00%> (ø)`
...loader/defaultcolumn/BaseDefaultColumnHandler.java	`63.26% <100.00%> (ø)`
... and 42 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 823aa07...ad6d167. Read the comment docs.

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/loader/SegmentDirectoryLoader.java

pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java

...gment-spi/src/main/java/org/apache/pinot/segment/spi/index/metadata/SegmentMetadataImpl.java

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

.../main/java/org/apache/pinot/segment/local/indexsegment/immutable/ImmutableSegmentLoader.java

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

npawar

lgtm

klsince · 2022-01-04T17:49:13Z

Thanks for the comments @npawar @jackjlli.

cc @siddharthteotia @Jackie-Jiang to help take a look as well, as you helped review the previous PR, a prep for this one. Thank you!

mcvsubbu

Still reviewing. Please hold merging

mcvsubbu · 2022-01-04T17:58:16Z

...ent-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/LoaderUtils.java

+
+  public static void createBackup(File indexDir)
+      throws IOException {
+    if (!indexDir.exists()) {


Why has this check been added?

replied below

mcvsubbu · 2022-01-04T18:00:33Z

...ent-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/LoaderUtils.java

+    Preconditions.checkState(indexDir.renameTo(segmentBackupDir),
+        "Failed to rename index directory: %s to segment backup directory: %s", indexDir, segmentBackupDir);
+    // Copy the backup dir back to proceed.
+    FileUtils.copyDirectory(segmentBackupDir, indexDir);


We used to rename the current directory and reload. Can you clarify why we have changed it to copy the segment instead?

renameTo happens at L172. The logic to create backup dir is not changed but moved to this util method.

Agree that the rename happens the same way, but prior to this PR, we just downloaded the segment. Looks like we are now copying after the rename (and then perhaps overwriting what we copied?)

It's on purpose to rename and then copy back, so that the subsequent segment preprocessing happens on the copy dir w/o affecting the ongoing queries, which if any are accessing the renamed dir which is kept intact even if segment reloading fails on the copy dir. As context, renameTo is an atomic operation.

prior to this PR, we just downloaded the segment

Previously, downloading doesn't always happens. It happens if segment CRC changes or forceDownload option is set to true, when one requests to reload segment (so downloading raw segment is relatively rare). In this PR, I made copy always happens, and if downloading happens, it overwrites the copy dir.

The change has been reverted. Just that copying backup directory back is done with SegmentDirectory.copyTo() now to decouple the logic with local segment directory.

mcvsubbu · 2022-01-04T18:12:36Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

-        indexDir = downloadSegment(segmentName, zkMetadata);
+      boolean shouldDownloadRawSegment = forceDownload || !hasSameCRC(zkMetadata, segmentMetadata);
+      if (shouldDownloadRawSegment && allowDownloadRawSegment(segmentName, zkMetadata)) {
+        downloadRawSegmentAndProcess(segmentName, indexLoadingConfig, zkMetadata, schema);


Can you clarify why we download from a "tier" (I assume this to mean a peer storage server that already hosts the segment), as opposed to always downloading from the master and keep things simple?

A server's tier backend is not peer server or deep store.

The refactoring in this PR would allow a server's tier backend to be local disk (the default impl) or remote fs (like s3). The segment from server's tier backend is not the raw segment from deep store or peer server, but the fully preprocessed segment ready to serve queries or to apply incremental changes like adding index.

When server uses remote tier backend, the File indexDir does not exist for the backup/restore flow, so I added the existence check at L166 ^

I referred to the design document https://docs.google.com/document/d/1Z4FLg3ezHpqvc6zhy0jR6Wi2OL8wLO_lRC6aLkskFgs/edit# and pinot documentation https://docs.pinot.apache.org/operators/operating-pinot/tiered-storage

I could not locate the meaning of a "tier backend". In order that the code be maintainable, please provide some clarification

thanks for the pointers. think I should make it clear that it means: a tier of servers with local or remote backend

@mcvsubbu tierBackend concept was already introduced last year (just updated the design doc to reflect it, sorry for not doing it when the change was added). Explanation regarding the new field added to the design too

What does it mean to have a "local" vs "non-local" tieredBackend? How is a segment expected to be different in a tierBackend? (Or, should we have this discussion in the design doc?) I am fine abstracting things as long as the comments in the code are clear about what the interfaces are supposed to do, and what layout of the cluster are we coding for

If I understand the questions correctly, lemme use an example to add some clarity:

The segments can move between tiers depends selection criteria like segment age (as said in the docs/wiki ^). The most recent segments may be kept in a tier where the servers in that tier use local disk; older segments may move to a tier where the servers use remote store to keep them. But in those tiers, the segments are preprocessed and ready for queries, just that some tier has low latency and some higher depending on servers' storage backend in those tiers.

As in a reply below, the segments in server's backend (or tier backend) are preprocessed with latest table config and schema and ready for queries.

To answer this further, Can you clarify why we download from a "tier" (I assume this to mean a peer storage server that already hosts the segment), as opposed to always downloading from the master and keep things simple? - there is a distinction being made between the master (raw) segment and the tier (fully preprocessed segment ready to serve queries), as mentioned by @klsince above. This distinction is primarily to help us future proof the implementations of lazy loading, where using the master copy of segment may suffice.

mcvsubbu · 2022-01-04T18:15:46Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

-          segmentTempDir);
-      FileUtils.deleteDirectory(segmentTempDir);
+      // Remove backup dir to mark the completion of reloading for local tier backend.
+      LoaderUtils.removeBackup(indexDir);


Can we keep the comments and renaming code here? Moving this to a method called "removeBackup" in a different class may lead to future modifications that may not keep the semantics of rename and remove (yes, you have moved the comments as well, but I think it is better to keep it local).

hmm.. do you mean to do renameTo outside the createBackup() and removeBackup() util methods?

I think rename, then copy or delete are pretty tightly coupled to finish one operation atomically, and I feel separating them would increase complexity for future maintenance.

btw, I extracted the two new util methods createBackup() and removeBackup() into LoaderUtils after seeing reloadFailureRecovery method already there. I kinda prefer to having those methods in one place, as they are all for the failure handling.

mcvsubbu

Still reviewing. Please hold off merge.

pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

...ent-local/src/main/java/org/apache/pinot/segment/local/segment/index/loader/LoaderUtils.java

mcvsubbu · 2022-01-05T00:46:27Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

+    SegmentDirectoryLoader segmentDirectoryLoader =
+        SegmentDirectoryLoaderRegistry.getSegmentDirectoryLoader(indexLoadingConfig.getSegmentDirectoryLoader());
+    File indexDir = getSegmentDataDir(segmentName);
+    segmentDirectoryLoader.download(indexDir, loaderContext);


It seems to me that the segment directory loader should take care of downloading from tier backend or deepstore, depending on some setting, perhaps in the loadercontext.

I am yet to understand what the difference is between a tier backend segment and the "raw" segment, though. Let me know what I am missing here.

Preprocessed segments (or tier backend segment) are put on server's storage backend (not using local disk here on purpose) to serve queries. SegmentDirectoryLoader interface knows how to load those preprocessed segments, essentially to initialize objects like SegmentDirectory and ColumnIndexDirectory to get ready for queries. Those interfaces abstract away the impl details and also where the preprocessed segments physically are.

It's on purpose not putting logic around deep store behind SegmentDirectoryLoader interface. As deep store is not designed as server's storage backend to hold preprocessed segments. Deep store keeps the raw segments, not preprocessed with latest table config and schema for queries.

the segment directory loader should take care of downloading from tier backend or deepstore, depending on some setting, perhaps in the loadercontext. - @klsince wdyt of this suggestion? Could we put download logic entirely behind the impl? Or is that hard to do because of the dependencies of downloadFromDeepstore?

Thanks for the feedbacks, @mcvsubbu ! We've proposed a new way to refactor the segment loading logic flow like below. pls review.

public void addOrReplaceSegment() { segdir = loader.load(); if (segdir == null || crc is diff) { download raw from deep store; preprocess(indexDir); } else if (not consistent with tbl cfg) { segdir.copyTo(indexDir); preprocess(indexDir); } if (preprocessed) { segdir = loader.load(); } return; }

below is the one used in PR right now as comparison.

public void addOrReplaceSegment() { segdir = loader.load(); if (segdir == null || crc is diff) { download raw from deep store; preprocess(indexDir); loader.upload(index) } else if (not consistent with tbl cfg) { loader.download(indexDir); preprocess(indexDir); loader.upload(index) } if (preprocessed) { segdir = loader.load(); } return; }

The doc does not convey any design details. It is simply a commentary on this PR. Please, include the text of the doc right here in the PR as a comment.

It is not clear (to me) what this PR is achieving. Is this a PR to just improve code readability? (you say "confusing" in your doc). Or, is this a PR to make Pinot more extensible for some purpose?

This is to make Pinot more extensible and future proofing for supporting cases like lazy loading and tiering. Improvement of readability is being done simply to make the review go easier, since that part of code was a lil complex to read to begin with..

Refactored with new way proposed above. Please review the updated code. Thanks!

mcvsubbu · 2022-01-07T19:47:08Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

-        LOGGER.info("Reload the local copy of segment: {} of table: {}", segmentName, _tableNameWithType);
-        FileUtils.copyDirectory(segmentBackupDir, indexDir);
+        LOGGER.info("Reload existing segment: {} of table: {}", segmentName, _tableNameWithType);
+        try (SegmentDirectory segmentDirectory = getSegmentDirectory(segmentName, indexLoadingConfig)) {


Why can't we use the same method as that in line 285 (getSegmentDataDir(segmentName)) ?
And also the FileUtils to copydir?

Replacing FileUtils.copyDirectory with SegmentDirectory.copyTo() so that the physical location of the source doesn't have to be a local seg dir. The physical location becomes transparent to the logic flow here.

Think I need to rename getSegmentDirectory() to sth like loadSegmentDirectory() for clarity, as it tries to create the SegmentDirectory object. The util method getSegmentDataDir just composes the file path to seg dir.

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

mcvsubbu · 2022-01-08T00:28:44Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

-        LOGGER.info("Reload the local copy of segment: {} of table: {}", segmentName, _tableNameWithType);
-        FileUtils.copyDirectory(segmentBackupDir, indexDir);
+        LOGGER.info("Reload existing segment: {} of table: {}", segmentName, _tableNameWithType);
+        try (SegmentDirectory segmentDirectory = initSegmentDirectory(segmentName, indexLoadingConfig)) {


This seems to be buggy. As I understand it, we seem to be copying from an empty dir to an empty dir, whereas we need to copy from the backupdir to the segment dir. Correct me if I am wrong.

right, we need to copy from backup dir, and that's implemented in SegmentLocalFSDirectory.copyTo() in this PR. That method is noop if indexDir exists already, or copies from backup dir to indexDir, keeping the original logic flow intact.

mcvsubbu · 2022-01-08T01:09:13Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

+    // still on the server but the metadata object has not been initialized yet. In this case,
+    // we should check if the segment exists on server and try to load it. If the segment does
+    // not exist or fails to get loaded, we download segment from deep store to load it again.
+    if (localMetadata == null && tryLoadExistingSegment(segmentName, indexLoadingConfig, zkMetadata)) {


tryLoadExistingSegment calls tryInitSegmentDirectory, which ends up loading the segment if it is locally present.
Yet, in line 510, we call load again. That seems to be a bug.

L510 ImmutableSegmentLoader.load(segmentDirectory, indexLoadingConfig, schema); uses the initialized SegmentDirectory object to create the ImmutableSegment object as the last step of the segment loading logic flow.

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

…ager so that it depends on SegmentDirectory interface instead of local seg dir to be more extensible

npawar · 2022-01-11T18:20:56Z

it looks like all major feedback has been addressed. if it’s only comments and naming that’s needing to change, can we merge this and add followups as needed? @snleee @mcvsubbu

snleee

Please fix the integration test. Thanks!

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/store/SegmentDirectory.java

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

klsince · 2022-01-11T20:00:49Z

Please fix the integration test. Thanks!

Yes, I'm investigating the failure after rebasing the latest master. It's running ok on my local. So I'm using a debug PR to dig on this. That debug PR is made off the latest master and with a few prints. But as you can see the failure has moved to another integ test case there. That made me wonder if the Github CI env got some issue. Not sure if you've got permission to help take a quick look at the CI env. Thank you @snleee.

snleee · 2022-01-11T20:13:02Z

@klsince I see. That may be a transient issue. I re-triggered the tests https://github.com/apache/pinot/actions/runs/1681129412

snleee

Otherwise LGTM

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

mcvsubbu · 2022-01-12T17:29:50Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

+
+    // Creates the SegmentDirectory object to access the segment metadata.
+    // The metadata is null if the segment doesn't exist yet.
+    SegmentDirectory segmentDirectory = tryInitSegmentDirectory(segmentName, indexLoadingConfig);


Can you verify that we don't see a warning in the normal case of loading a new segment? It seems to me that the following steps will happen.

addOrReplace() -> tryLoadExistingSegment() -> tryInitSegmentDirectory() -> initSegmentDirectory()

The last method catches and exception and logs a warning.

There should not be any warnings in the normal case.

You're right about the call stack ^ when loading a branch new segment.

When server loads a brand new segment, there is no indexDir on disk yet, so SegmentLocalFSDirectory is initialized with an empty dir (no warning emit). But with an empty SegmentLocalFSDirectory object, we get to L502/L508 to return false from tryLoadExistingSegment() method. Then, the addOrReplaceSegment() method continues to download the new segment from deep store to finish loading.

This keeps the original logic flow intact. If you check the original code, it does recoverSegmentQuietly and loadSegmentQuietly, and if they fails, then go to download segment from deep store to finish loading.

mcvsubbu · 2022-01-12T17:31:04Z

Overall, I am a -1 on this PR. It has taken what was a bit complex but at least intuitive to read code to a completely confusing piece of code.

mcvsubbu

I think a factory for creating the appropriate segmentDirectory may help. And/or , an interface method at SegmentDirectory level of something like: boolean isLocal()

mcvsubbu · 2022-01-13T19:54:10Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

+    FileUtils.deleteDirectory(segmentTempDir);
+  }
+
+  private boolean tryLoadExistingSegment(String segmentName, IndexLoadingConfig indexLoadingConfig,


Can u add a comment here stating the conditions when it returns true or false. As I understand, true means it existed locally and has been loaded into memory. false means it may exist locally with a different CRC or it may not exist at all. Is the SegmentDirectory always closed upon return?

Will add comment, and for the questions:

It also returns false if loading fails with exception. As before, upon exception to load locally existed segment, we should fall back to download the segment from deep store to finish the loading.

SegmentDirectory object is closed when the method has to return false.

mcvsubbu · 2022-01-13T19:55:21Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

    } else {
-      LOGGER.info("Download segment: {} of table: {} as local crc: {} mismatches remote crc: {}.", segmentName,
+      LOGGER.info("Download segment: {} of table: {} as crc changes from: {} to: {}", segmentName,


will this log message happen twice when crc changes? is there a way to avoid that? It is already logged in tryLoadExisting() ... method

This is a good question. Either of them gets logged in one method call, but not both. Because the two CRC checks have used CRC values from different places when comparing with the CRC in SegmentZKMetadata.

Here the localMetadata object is not null, and the CRC from it has been used to compare with the CRC in SegmentZkMetadata (L342). The tryLoadExistingSegment() method is called when localMetadata is null. And the CRC is retrieved from segment potentially exists locally (but not loaded yet) to compare with the CRC in SegmentZkMetadata.

mcvsubbu · 2022-01-13T20:10:42Z

pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java

@@ -307,6 +306,30 @@ private void reloadSegment(String tableNameWithType, SegmentMetadata segmentMeta
    }
  }

+  /**
+   * Try to reload a mutable segment.
+   * @return true if the segment is mutable; false if the segment is immutable.


Suggested change

* @return true if the segment is mutable; false if the segment is immutable.

* @return true if the segment is mutable and loaded; false if the segment is immutable.

mcvsubbu · 2022-01-13T20:11:58Z

pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java

+   * @return true if the segment is mutable; false if the segment is immutable.
+   */
+  @VisibleForTesting
+  boolean reloadMutableSegment(String tableNameWithType, String segmentName,


Maybe rename to reloadIfMutabelSegment() ?

still prefer reloadMutableSegment as you suggested early on.

mcvsubbu · 2022-01-13T21:32:21Z

This the best I could do as far as suggestions. Please feel free to merge after addressing my minor comments in the latest review. The comment about having a factory is for future, esp. if you are planning more changes in this area due to a non-local segment directory. If we discover issues in production, we may need to undo or re-work some of the changes. Like I mentioned in a previous comment as well, a lot of these are best hidden behind your implementation of SegmentDirectory.

klsince · 2022-01-13T22:23:08Z

I think a factory for creating the appropriate segmentDirectory may help. And/or , an interface method at SegmentDirectory level of something like: boolean isLocal()

Yes, currently, there is a factory method: SegmentDirectoryLoaderRegistry.getSegmentDirectoryLoader, which returns certain type of SegmentDirectoryLoader to initialize specific SegmentDirectory objects.

I'm not so sure about SegmentDirectory.isLocal(). This PR didn't require it, as the segment location is made transparent behind SegmentDirectory interface. It may enable other future extensions and would love to revisit it.

klsince · 2022-01-14T00:15:30Z

hi @mcvsubbu thanks a lot for the feedbacks! I've replied your latest comments. Please take a look, and if they look good, could you help lift the 'requested changes' and I'll merge it for now. Thanks!

mcvsubbu · 2022-01-14T00:32:40Z

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java

+   *
+   * @return true if the segment still exists on server, its CRC is still same with the
+   * one in SegmentZKMetadata and is loaded into memory successfully; false if it doesn't
+   * exist on the server, its CRC has changed, or it fails to be loaded.


please add the segmentdirectory is closed when it is false, open otherwise

mcvsubbu

minor comment to be added

npawar reviewed Jan 3, 2022

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java Outdated Show resolved Hide resolved

jackjlli reviewed Jan 3, 2022

View reviewed changes

klsince force-pushed the decouple_segment_load_with_local_file branch from 95f97a6 to f76a05c Compare January 4, 2022 00:17

npawar approved these changes Jan 4, 2022

View reviewed changes

mcvsubbu reviewed Jan 4, 2022

View reviewed changes

mcvsubbu requested changes Jan 4, 2022

View reviewed changes

klsince force-pushed the decouple_segment_load_with_local_file branch from 03c234b to 8bc9da6 Compare January 5, 2022 00:33

mcvsubbu reviewed Jan 5, 2022

View reviewed changes

klsince changed the title ~~extend SegmentDirectoryLoader interface and refactor BaseTableDataManager~~ refactor segment loading logic in BaseTableDataManager to decouple it with local segment directory Jan 7, 2022

klsince force-pushed the decouple_segment_load_with_local_file branch 3 times, most recently from cf77dab to a240fe4 Compare January 7, 2022 17:10

mcvsubbu reviewed Jan 7, 2022

View reviewed changes

mcvsubbu reviewed Jan 8, 2022

View reviewed changes

klsince requested a review from mcvsubbu January 10, 2022 20:24

snleee reviewed Jan 10, 2022

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java Show resolved Hide resolved

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java Show resolved Hide resolved

Xiaobing Li added 8 commits January 10, 2022 17:22

extend SegmentDirectoryLoader interface and refactor BaseTableDataMan…

19e0223

…ager so that it depends on SegmentDirectory interface instead of local seg dir to be more extensible

add license header

a9c8551

address comments

3e71b03

rebase and fix UT

9ffb206

cr - refine comments, namings and logs, and refactor a few method

f5f8c98

refactor BaseTableDataMgr with SegmentDirectory.copyTo

9b6ccfd

refine naming and comments

6f4b6ca

close segdir before early return

745daa4

klsince force-pushed the decouple_segment_load_with_local_file branch 2 times, most recently from b47697f to e801de0 Compare January 11, 2022 04:03

rebase master

638a89a

klsince force-pushed the decouple_segment_load_with_local_file branch from e801de0 to 638a89a Compare January 11, 2022 06:28

snleee reviewed Jan 11, 2022

View reviewed changes

pinot-segment-spi/src/main/java/org/apache/pinot/segment/spi/store/SegmentDirectory.java Show resolved Hide resolved

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java Show resolved Hide resolved

snleee approved these changes Jan 12, 2022

View reviewed changes

pinot-core/src/main/java/org/apache/pinot/core/data/manager/BaseTableDataManager.java Show resolved Hide resolved

comments for clarity

ac41900

mcvsubbu reviewed Jan 12, 2022

View reviewed changes

mcvsubbu reviewed Jan 13, 2022

View reviewed changes

refine comments

46717cc

mcvsubbu reviewed Jan 14, 2022

View reviewed changes

mcvsubbu approved these changes Jan 14, 2022

View reviewed changes

add more context in comments

ad6d167

npawar merged commit e31e6f9 into apache:master Jan 14, 2022

	* @return true if the segment is mutable; false if the segment is immutable.
	* @return true if the segment is mutable and loaded; false if the segment is immutable.

refactor segment loading logic in BaseTableDataManager to decouple it with local segment directory #7969

refactor segment loading logic in BaseTableDataManager to decouple it with local segment directory #7969

Conversation

klsince commented Dec 30, 2021 • edited Loading

Description

Upgrade Notes

Release Notes

Documentation

codecov-commenter commented Dec 30, 2021 • edited Loading

Codecov Report

npawar left a comment

Choose a reason for hiding this comment

klsince commented Jan 4, 2022

mcvsubbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcvsubbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klsince Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcvsubbu Jan 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npawar commented Jan 11, 2022

snleee left a comment

Choose a reason for hiding this comment

klsince commented Jan 11, 2022 • edited Loading

snleee commented Jan 11, 2022 • edited Loading

snleee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcvsubbu commented Jan 12, 2022

mcvsubbu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcvsubbu commented Jan 13, 2022

klsince commented Jan 13, 2022

klsince commented Jan 14, 2022

Choose a reason for hiding this comment

mcvsubbu left a comment

Choose a reason for hiding this comment

klsince commented Dec 30, 2021 •

edited

Loading

codecov-commenter commented Dec 30, 2021 •

edited

Loading

klsince Jan 6, 2022 •

edited

Loading

mcvsubbu Jan 7, 2022 •

edited

Loading

klsince commented Jan 11, 2022 •

edited

Loading

snleee commented Jan 11, 2022 •

edited

Loading