-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-32381][CORE][SQL] Move and refactor parallel listing & non-location sensitive listing to core #29471
[SPARK-32381][CORE][SQL] Move and refactor parallel listing & non-location sensitive listing to core #29471
Changes from 27 commits
2d6add1
02ea25d
fded394
f2fdcd7
37253da
e6eee1d
0492bee
138e14a
dace630
20586d3
7bb0770
4eb770a
8a5fd8b
d85c5a4
3b0bf18
8b7a2fa
e0ec9a6
b1bf5e9
5529047
6f5c7e5
43fc5a0
1f9bbbc
0ab104d
2186a66
549f335
2b1aacd
f5e9581
86c2013
bfa37cc
2d8e64d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,360 @@ | ||
/* | ||
* Licensed to the Apache Software Foundation (ASF) under one or more | ||
* contributor license agreements. See the NOTICE file distributed with | ||
* this work for additional information regarding copyright ownership. | ||
* The ASF licenses this file to You under the Apache License, Version 2.0 | ||
* (the "License"); you may not use this file except in compliance with | ||
* the License. You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
package org.apache.spark.util | ||
|
||
import java.io.FileNotFoundException | ||
|
||
import scala.collection.mutable | ||
|
||
import org.apache.hadoop.conf.Configuration | ||
import org.apache.hadoop.fs._ | ||
import org.apache.hadoop.fs.viewfs.ViewFileSystem | ||
import org.apache.hadoop.hdfs.DistributedFileSystem | ||
|
||
import org.apache.spark._ | ||
import org.apache.spark.annotation.Private | ||
import org.apache.spark.internal.Logging | ||
import org.apache.spark.metrics.source.HiveCatalogMetrics | ||
|
||
/** | ||
* Utility functions to simplify and speed-up file listing. | ||
*/ | ||
@Private | ||
object HadoopFSUtils extends Logging { | ||
sunchao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/** | ||
* Lists a collection of paths recursively. Picks the listing strategy adaptively depending | ||
* on the number of paths to list. | ||
* | ||
* This may only be called on the driver. | ||
sunchao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* @param sc Spark context used to run parallel listing. | ||
* @param paths Input paths to list | ||
* @param hadoopConf Hadoop configuration | ||
* @param filter Path filter used to exclude leaf files from result | ||
* @param areSQLRootPaths Whether the input paths are SQL root paths | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add more few words for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this is unfortunate. I think this parameter doesn't have to be visible to the callers though as it is set to true on the initial call and false on subsequent recursive calls. We can potentially add another overloaded method without this parameter and make this one private. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like that idea @sunchao |
||
* @param ignoreMissingFiles Ignore missing files that happen during recursive listing | ||
* (e.g., due to race conditions) | ||
* @param ignoreLocality Whether to fetch data locality info when listing leaf files. If false, | ||
* this will return `FileStatus` without `BlockLocation` info. | ||
* @param parallelismThreshold The threshold to enable parallelism. If the number of input paths | ||
* is smaller than this value, this will fallback to use | ||
* sequential listing. | ||
* @param parallelismMax The maximum parallelism for listing. If the number of input paths is | ||
* larger than this value, parallelism will be throttled to this value | ||
* to avoid generating too many tasks. | ||
sunchao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* @param filterFun Optional predicate on the leaf files. Files who failed the check will be | ||
* excluded from the results | ||
* @return for each input path, the set of discovered files for the path | ||
*/ | ||
def parallelListLeafFiles( | ||
sc: SparkContext, | ||
paths: Seq[Path], | ||
hadoopConf: Configuration, | ||
filter: PathFilter, | ||
areSQLRootPaths: Boolean, | ||
ignoreMissingFiles: Boolean, | ||
ignoreLocality: Boolean, | ||
parallelismThreshold: Int, | ||
parallelismMax: Int, | ||
filterFun: Option[String => Boolean] = None): Seq[(Path, Seq[FileStatus])] = { | ||
|
||
// Short-circuits parallel listing when serial listing is likely to be faster. | ||
if (paths.size <= parallelismThreshold) { | ||
return paths.map { path => | ||
val leafFiles = listLeafFiles( | ||
path, | ||
hadoopConf, | ||
filter, | ||
Some(sc), | ||
ignoreMissingFiles = ignoreMissingFiles, | ||
ignoreLocality = ignoreLocality, | ||
isSQLRootPath = areSQLRootPaths, | ||
parallelismThreshold = parallelismThreshold, | ||
parallelismDefault = parallelismMax, | ||
filterFun = filterFun) | ||
(path, leafFiles) | ||
} | ||
} | ||
|
||
logInfo(s"Listing leaf files and directories in parallel under ${paths.length} paths." + | ||
s" The first several paths are: ${paths.take(10).mkString(", ")}.") | ||
HiveCatalogMetrics.incrementParallelListingJobCount(1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Put There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. It seems inappropriate here. Not sure what is the best way to plugin Hive/SQL metrics here but I'll think over it later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could pass in a callback for listing metrics? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or call this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the problem with that is that if we have a root directory with many sub-directories in it, we may initially choose to do non-parallel listing and then as the sub directories build up switch to parallel listing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah move this to SQL will not work - I think we can perhaps add a callback later just like @holdenk suggested (if you don't mind leaving it here in this PR). On the other hand, I think the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A callback sounds good. We can do it later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good to me, @sunchao want to file a JIRA for switching this to a callback? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure filed https://issues.apache.org/jira/browse/SPARK-32880 for the follow-ups. |
||
|
||
val serializableConfiguration = new SerializableConfiguration(hadoopConf) | ||
sunchao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
val serializedPaths = paths.map(_.toString) | ||
|
||
// Set the number of parallelism to prevent following file listing from generating many tasks | ||
// in case of large #defaultParallelism. | ||
val numParallelism = Math.min(paths.size, parallelismMax) | ||
|
||
val previousJobDescription = sc.getLocalProperty(SparkContext.SPARK_JOB_DESCRIPTION) | ||
val statusMap = try { | ||
val description = paths.size match { | ||
case 0 => | ||
"Listing leaf files and directories 0 paths" | ||
case 1 => | ||
s"Listing leaf files and directories for 1 path:<br/>${paths(0)}" | ||
case s => | ||
s"Listing leaf files and directories for $s paths:<br/>${paths(0)}, ..." | ||
} | ||
sc.setJobDescription(description) | ||
sc | ||
.parallelize(serializedPaths, numParallelism) | ||
.mapPartitions { pathStrings => | ||
val hadoopConf = serializableConfiguration.value | ||
pathStrings.map(new Path(_)).toSeq.map { path => | ||
val leafFiles = listLeafFiles( | ||
path = path, | ||
hadoopConf = hadoopConf, | ||
filter = filter, | ||
contextOpt = None, // Can't execute parallel scans on workers | ||
ignoreMissingFiles = ignoreMissingFiles, | ||
ignoreLocality = ignoreLocality, | ||
isSQLRootPath = areSQLRootPaths, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. shall we have a consistent name here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is copied from the current impl in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the is/are is because listLeafFiles takes a single name and this method takes in a list of files. |
||
filterFun = filterFun, | ||
parallelismThreshold = Int.MaxValue, | ||
parallelismDefault = 0) | ||
(path, leafFiles) | ||
}.iterator | ||
}.map { case (path, statuses) => | ||
val serializableStatuses = statuses.map { status => | ||
// Turn FileStatus into SerializableFileStatus so we can send it back to the driver | ||
val blockLocations = status match { | ||
case f: LocatedFileStatus => | ||
f.getBlockLocations.map { loc => | ||
SerializableBlockLocation( | ||
loc.getNames, | ||
loc.getHosts, | ||
loc.getOffset, | ||
loc.getLength) | ||
} | ||
|
||
case _ => | ||
Array.empty[SerializableBlockLocation] | ||
} | ||
|
||
SerializableFileStatus( | ||
status.getPath.toString, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. recent hadoop builds declare path Serializable,. FileStatus should be too, but there's always the risk some subclass isn't There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I plan to remove this in a followup, and since There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
"should"; there's no tests for it, and nothing in the java language which stops you adding references to non-serializable objects. Exception has this exact issue |
||
status.getLen, | ||
status.isDirectory, | ||
status.getReplication, | ||
status.getBlockSize, | ||
status.getModificationTime, | ||
status.getAccessTime, | ||
blockLocations) | ||
} | ||
(path.toString, serializableStatuses) | ||
}.collect() | ||
} finally { | ||
sc.setJobDescription(previousJobDescription) | ||
} | ||
|
||
// turn SerializableFileStatus back to Status | ||
statusMap.map { case (path, serializableStatuses) => | ||
val statuses = serializableStatuses.map { f => | ||
val blockLocations = f.blockLocations.map { loc => | ||
new BlockLocation(loc.names, loc.hosts, loc.offset, loc.length) | ||
} | ||
new LocatedFileStatus( | ||
new FileStatus( | ||
f.length, f.isDir, f.blockReplication, f.blockSize, f.modificationTime, | ||
new Path(f.path)), | ||
blockLocations) | ||
} | ||
(new Path(path), statuses) | ||
} | ||
} | ||
|
||
// scalastyle:off argcount | ||
/** | ||
* Lists a single filesystem path recursively. If a `SparkContext`` object is specified, this | ||
sunchao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* function may launch Spark jobs to parallelize listing based on parallelismThreshold. | ||
sunchao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
* | ||
* If sessionOpt is None, this may be called on executors. | ||
* | ||
* @return all children of path that match the specified filter. | ||
*/ | ||
private def listLeafFiles( | ||
path: Path, | ||
hadoopConf: Configuration, | ||
filter: PathFilter, | ||
contextOpt: Option[SparkContext], | ||
ignoreMissingFiles: Boolean, | ||
ignoreLocality: Boolean, | ||
isSQLRootPath: Boolean, | ||
filterFun: Option[String => Boolean], | ||
parallelismThreshold: Int, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems a new parameter that did not exist before. Why do we need this? If people want parallelized listing, people can invoke There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is because |
||
parallelismDefault: Int): Seq[FileStatus] = { | ||
|
||
logTrace(s"Listing $path") | ||
val fs = path.getFileSystem(hadoopConf) | ||
|
||
// Note that statuses only include FileStatus for the files and dirs directly under path, | ||
// and does not include anything else recursively. | ||
val statuses: Array[FileStatus] = try { | ||
fs match { | ||
// DistributedFileSystem overrides listLocatedStatus to make 1 single call to namenode | ||
// to retrieve the file status with the file block location. The reason to still fallback | ||
// to listStatus is because the default implementation would potentially throw a | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd question that -and its much better to use the incremental listLocatedStatus for better performance on paged object store listings, especially if you can do useful stuff while it takes place There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, do you think we can add something to CommonPathCapabilities for this? so that we don't have to enumerate all these here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would just switch to the incremental one everywhere HDFS likes it because when you have many, many files in a path they can release the lock on the NN; for object stores they always have to page in (s3, azure, GCS)....which gives the implementors an opportunity to move from the default implementation to exposing the incremental one. Add this and we can just let the relevant teams know. w.r.t CommonPathCapabilities, I suppose we could add one which declares that the listing ops are paged. But do you really want code to try and be that clever? I'm trying to use that feature to mark up
If you want a performance option, we could add one. BTW, PathCapabilities is in hadoop-3.2.x now, will be in next release. I might do it for 3.1 too...it makes a good way to programmatically/CLI probe for s3a dir marker policy, see. Also, cloudstore has some CLI commands for the list calls (and pathcapabilities) to help explore what's going on, including, on s3a, listing cost in #of HTTP requests. Worth playing with to see what is good/bad, though as Mukund has been doing lots of work on 3.3.x s3a list, it will look worse than you'd expect on a dir treewalk, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think changing this in a follow up PR sounds fine to me. I'd like to us to use the faster method by default and fall back on exception, but that could be a follow on. Want to file a JIRA for broadening the scope of using the faster method? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. w.r.t the faster (incremental) calls, yes, something to consider next. At the very least, you will be able to collect and report/aggregate stats. For example, here is me getting the stats on a large/deep list just by calling toString on the RemoteIterator after it's done its work
Now, who wouldn't like to know things like that. And ideally, collect across threads and merge back in. As an aside, looked at {{org.apache.hadoop.mapred.LocatedFileStatusFetcher}}. This does multithreaded status fetching and collects those stats. Although it's tagged Private, I've noticed Parquet uses it so my next PR will convert to public/evolving and document the fact. If you could use that, we could look @ evolving it better, especially returning a RemoteIterator of results which we could incrementally fill in across threads rather than block for the final results. Anything which makes it possible for app code to process data (read footers, etc) while the listing goes on has significant benefit in the World of Object Stores There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, stats will be nice. Looking forward to that 👍 (is it tracked by this PR?) w.r.t Instead of multiple threads, Spark currently use a distributed job so I'm not sure whether there will be any regression by doing that, but we can explore this later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'd worry about the effect of moving the package as is. It'd have to be a copy and paste, or move and subclass There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @sunchao -yes, that's the stats API. Now is the time to review it and tell me where it doesn't suit your (perceived future) needs. I've been playing with a Parquet branch which uses it, not in Spark itself. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the info @steveloughran . I'll check out the PR (it is a very big one though). |
||
// FileNotFoundException which is better handled by doing the lookups manually below. | ||
case (_: DistributedFileSystem | _: ViewFileSystem) if !ignoreLocality => | ||
val remoteIter = fs.listLocatedStatus(path) | ||
new Iterator[LocatedFileStatus]() { | ||
def next(): LocatedFileStatus = remoteIter.next | ||
def hasNext(): Boolean = remoteIter.hasNext | ||
}.toArray | ||
case _ => fs.listStatus(path) | ||
} | ||
} catch { | ||
// If we are listing a root path for SQL (e.g. a top level directory of a table), we need to | ||
// ignore FileNotFoundExceptions during this root level of the listing because | ||
// | ||
// (a) certain code paths might construct an InMemoryFileIndex with root paths that | ||
// might not exist (i.e. not all callers are guaranteed to have checked | ||
// path existence prior to constructing InMemoryFileIndex) and, | ||
// (b) we need to ignore deleted root paths during REFRESH TABLE, otherwise we break | ||
// existing behavior and break the ability drop SessionCatalog tables when tables' | ||
// root directories have been deleted (which breaks a number of Spark's own tests). | ||
// | ||
// If we are NOT listing a root path then a FileNotFoundException here means that the | ||
// directory was present in a previous level of file listing but is absent in this | ||
// listing, likely indicating a race condition (e.g. concurrent table overwrite or S3 | ||
// list inconsistency). | ||
// | ||
// The trade-off in supporting existing behaviors / use-cases is that we won't be | ||
// able to detect race conditions involving root paths being deleted during | ||
// InMemoryFileIndex construction. However, it's still a net improvement to detect and | ||
// fail-fast on the non-root cases. For more info see the SPARK-27676 review discussion. | ||
case _: FileNotFoundException if isSQLRootPath || ignoreMissingFiles => | ||
logWarning(s"The directory $path was not found. Was it deleted very recently?") | ||
Array.empty[FileStatus] | ||
} | ||
|
||
def doFilter(statuses: Array[FileStatus]) = filterFun match { | ||
case Some(shouldFilterOut) => | ||
statuses.filterNot(status => shouldFilterOut(status.getPath.getName)) | ||
case None => | ||
statuses | ||
} | ||
|
||
val filteredStatuses = doFilter(statuses) | ||
val allLeafStatuses = { | ||
val (dirs, topLevelFiles) = filteredStatuses.partition(_.isDirectory) | ||
val nestedFiles: Seq[FileStatus] = contextOpt match { | ||
case Some(context) if dirs.size > parallelismThreshold => | ||
parallelListLeafFiles( | ||
context, | ||
dirs.map(_.getPath), | ||
hadoopConf = hadoopConf, | ||
filter = filter, | ||
areSQLRootPaths = false, | ||
ignoreMissingFiles = ignoreMissingFiles, | ||
ignoreLocality = ignoreLocality, | ||
filterFun = filterFun, | ||
parallelismThreshold = parallelismThreshold, | ||
parallelismMax = parallelismDefault | ||
).flatMap(_._2) | ||
case _ => | ||
dirs.flatMap { dir => | ||
listLeafFiles( | ||
path = dir.getPath, | ||
hadoopConf = hadoopConf, | ||
filter = filter, | ||
contextOpt = contextOpt, | ||
ignoreMissingFiles = ignoreMissingFiles, | ||
ignoreLocality = ignoreLocality, | ||
isSQLRootPath = false, | ||
filterFun = filterFun, | ||
parallelismThreshold = parallelismThreshold, | ||
parallelismDefault = parallelismDefault) | ||
} | ||
} | ||
val allFiles = topLevelFiles ++ nestedFiles | ||
if (filter != null) allFiles.filter(f => filter.accept(f.getPath)) else allFiles | ||
} | ||
|
||
val missingFiles = mutable.ArrayBuffer.empty[String] | ||
val filteredLeafStatuses = doFilter(allLeafStatuses) | ||
val resolvedLeafStatuses = filteredLeafStatuses.flatMap { | ||
case f: LocatedFileStatus => | ||
Some(f) | ||
|
||
// NOTE: | ||
// | ||
// - Although S3/S3A/S3N file system can be quite slow for remote file metadata | ||
// operations, calling `getFileBlockLocations` does no harm here since these file system | ||
// implementations don't actually issue RPC for this method. | ||
// | ||
// - Here we are calling `getFileBlockLocations` in a sequential manner, but it should not | ||
// be a big deal since we always use to `parallelListLeafFiles` when the number of | ||
// paths exceeds threshold. | ||
case f if !ignoreLocality => | ||
// The other constructor of LocatedFileStatus will call FileStatus.getPermission(), | ||
// which is very slow on some file system (RawLocalFileSystem, which is launch a | ||
// subprocess and parse the stdout). | ||
try { | ||
val locations = fs.getFileBlockLocations(f, 0, f.getLen).map { loc => | ||
// Store BlockLocation objects to consume less memory | ||
if (loc.getClass == classOf[BlockLocation]) { | ||
loc | ||
} else { | ||
new BlockLocation(loc.getNames, loc.getHosts, loc.getOffset, loc.getLength) | ||
} | ||
} | ||
val lfs = new LocatedFileStatus(f.getLen, f.isDirectory, f.getReplication, f.getBlockSize, | ||
f.getModificationTime, 0, null, null, null, null, f.getPath, locations) | ||
if (f.isSymlink) { | ||
lfs.setSymlink(f.getSymlink) | ||
} | ||
Some(lfs) | ||
} catch { | ||
case _: FileNotFoundException if ignoreMissingFiles => | ||
missingFiles += f.getPath.toString | ||
None | ||
} | ||
|
||
case f => Some(f) | ||
} | ||
|
||
if (missingFiles.nonEmpty) { | ||
logWarning( | ||
s"the following files were missing during file scan:\n ${missingFiles.mkString("\n ")}") | ||
} | ||
|
||
resolvedLeafStatuses.toSeq | ||
sunchao marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
// scalastyle:on argcount | ||
|
||
/** A serializable variant of HDFS's BlockLocation. */ | ||
private case class SerializableBlockLocation( | ||
names: Array[String], | ||
hosts: Array[String], | ||
offset: Long, | ||
length: Long) | ||
|
||
/** A serializable variant of HDFS's FileStatus. */ | ||
private case class SerializableFileStatus( | ||
path: String, | ||
length: Long, | ||
isDir: Boolean, | ||
blockReplication: Short, | ||
blockSize: Long, | ||
modificationTime: Long, | ||
accessTime: Long, | ||
blockLocations: Array[SerializableBlockLocation]) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Private maybe seems a bit too restrictive in scope, what about DeveloperAPI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zsxwing suggested to make this private in the meanwhile and change it after the follow-up PR is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. I suggested to make this private. I think these APIs are not ready to expose yet. For example, 10 parameters in a method is not user friendly. It's better to design a better API for this.