Add resultIndex to session manager extension #689
Conversation
noCharger
requested review from
dai-chen,
rupal-bq,
vmmusings,
penghuo,
seankao-az,
anirudha,
kaituo and
YANG-DB
as code owners
| @@ -983,7 +983,8 @@ object FlintREPL extends Logging with FlintJobExecutor { | |||
| resultIndexOption: Option[String]): SessionManager = { | |||
| instantiate( | |||
| new SessionManagerImpl(spark, resultIndexOption), | |||
| spark.conf.get(FlintSparkConf.CUSTOM_SESSION_MANAGER.key, "")) | |||
| spark.conf.get(FlintSparkConf.CUSTOM_SESSION_MANAGER.key, ""), | |||
| resultIndexOption) | |||
There was a problem hiding this comment.
Just to confirm: custom session manager constructor in Java can accept Scala's Option[String]?
There was a problem hiding this comment.
Java project needs to add Scala's library. For example
import scala.Option;
import scala.Some;
import scala.None$;
MyClass myClass = new MyClass(Some.apply("Hello")); // using Scala's Some
But I think it makes more sense to convert this as String for ease of use
|
|
||
| class QueryResultWriterImpl(context: Map[String, Any]) extends QueryResultWriter with Logging { | ||
|
|
||
| private val resultIndex = context("resultIndex").asInstanceOf[String] | ||
| private val osClient = context("osClient").asInstanceOf[OSClient] | ||
| private val osClient = new OSClient(FlintSparkConf().flintOptions()) |
There was a problem hiding this comment.
Sorry could you elaborate this change?
There was a problem hiding this comment.
Default session manager impl keeps an OSClient in context map and expose it to OpenSearch QueryResultWriter. For the custom session manager, it doesn't make sense to do so to work with OpenSearch QueryResultWriter.
There was a problem hiding this comment.
A bit unintuitive.
Could we add a comment here? Because if we're just reading the code (our OpenSearch implementations) we might wonder why we're not using the osClient available in context.
If I'm understanding correctly, here we're not using context("osClient") because we can't rely on the session manager implementation to put osClient in context?
There was a problem hiding this comment.
sure, comment added
noCharger
force-pushed
the
add-result-index
branch
from
4ea2aa0 to
68373aa
Compare
Signed-off-by: Louis Chu <[email protected]>
noCharger
force-pushed
the
add-result-index
branch
from
68373aa to
2cc332d
Compare
Signed-off-by: Louis Chu <[email protected]> (cherry picked from commit f742a65) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Louis Chu <[email protected]> (cherry picked from commit f742a65) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit f742a65) Signed-off-by: Louis Chu <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
(cherry picked from commit f742a65) Signed-off-by: Louis Chu <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Add resultIndex to session manager extension constructor. In this case custom session manager can work with default QueryResultWriter to persist result to OpenSearch
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.