[HUDI-8637] Refactor broadcast to a more generic reader context factory #13148
Conversation
| CompactionOperation operation, | ||
| String instantTime, | ||
| Option<EngineBroadcastManager> broadcastManagerOpt) throws IOException { | ||
| HoodieWriteConfig writeConfig, |
There was a problem hiding this comment.
let's fix the indentation.
|
|
||
| import java.io.Serializable; | ||
|
|
||
| public interface ReaderContextFactory<T> extends Serializable { |
There was a problem hiding this comment.
Can we add some doc to it.
|
|
||
| @Override | ||
| public void close() { | ||
| this.schemaToVersionId.clear(); |
There was a problem hiding this comment.
seems not releated with the change?
There was a problem hiding this comment.
It's part of the cleanup. Generally there seems to be over use of closeable in some new code. This cleanup will make it easier to be consistent with enforcing that all closeables are closed.
There was a problem hiding this comment.
If the cleanup should be part of a follow up PR, then I can remove the changes related to this
|
|
||
| private final transient HoodieEngineContext context; | ||
| private final transient HoodieTableMetaClient metaClient; | ||
| class SparkReaderContextFactory implements ReaderContextFactory<InternalRow> { |
| @@ -319,9 +319,6 @@ public void close() throws IOException { | |||
| if (recordBuffer != null) { | |||
| recordBuffer.close(); | |||
| } | |||
| if (readerContext != null) { | |||
| readerContext.close(); | |||
There was a problem hiding this comment.
what's the problem here with the close?
There was a problem hiding this comment.
The reader context doesn't have anything to close so there is no need for this line currently and no need to make the class implement closeable
There was a problem hiding this comment.
you mean the schema cache is small?
There was a problem hiding this comment.
No, the schema cache does not actually have anything that requires closing. The cache object is only referenced from the context that creates it. Once that context is no longer referenced, the cache is eligible for GC.
Change Logs
General refactoring to move away from the spark specific broadcast nomenclature for ReaderContext to a serializable factory that serves basically the same purpose. Main changes:
Impact
This is mainly cleanup and setup work for making the FileGroupReader the default throughout the codebase.
Risk level (write none, low medium or high below)
None
Documentation Update
Describe any necessary documentation update if there is any new feature, config, or user-facing change. If not, put "none".
ticket number here and follow the instruction to make
changes to the website.
Contributor's checklist