You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@griffin.apache.org by grant-xuexu <gi...@git.apache.org> on 2018/08/15 15:06:54 UTC
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
GitHub user grant-xuexu opened a pull request:
https://github.com/apache/incubator-griffin/pull/396
set the fields in configuration classes private
in configuration case classes, get*** functions are defined to access to the fields, so for consistency, make the fields private
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/grant-xuexu/incubator-griffin configuration
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-griffin/pull/396.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #396
----
commit 23ff42a265fd1298f38205316153e959999d4037
Author: grant-xuexu <gr...@...>
Date: 2018-08-15T14:59:00Z
set the fields in configuration classes private
----
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Posted by grant-xuexu <gi...@git.apache.org>.
Github user grant-xuexu commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210924616
--- Diff: measure/src/main/scala/org/apache/griffin/measure/datasource/connector/DataConnectorFactory.scala ---
@@ -91,11 +91,12 @@ object DataConnectorFactory extends Loggable {
): KafkaStreamingDataConnector = {
val KeyType = "key.type"
val ValueType = "value.type"
- val config = dcParam.config
+ val config = dcParam.getConfig
val keyType = config.getOrElse(KeyType, "java.lang.String").toString
val valueType = config.getOrElse(ValueType, "java.lang.String").toString
- (getClassTag(keyType), getClassTag(valueType)) match {
- case (ClassTag(k: Class[String]), ClassTag(v: Class[String])) => {
+
+ (keyType, valueType) match {
+ case ("java.lang.String", "java.lang.String") => {
KafkaStreamingStringDataConnector(sparkSession, ssc, dcParam, tmstCache, streamingCacheClientOpt)
--- End diff --
Or we could move the type checking logic to configuration validation
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Posted by guoyuepeng <gi...@git.apache.org>.
Github user guoyuepeng commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210854573
--- Diff: measure/src/main/scala/org/apache/griffin/measure/step/builder/dsl/expr/TreeNode.scala ---
@@ -18,28 +18,42 @@ under the License.
*/
package org.apache.griffin.measure.step.builder.dsl.expr
+import scala.reflect.ClassTag
+
trait TreeNode extends Serializable {
var children = Seq[TreeNode]()
def addChild(expr: TreeNode) = { children :+= expr }
def addChildren(exprs: Seq[TreeNode]) = { children ++= exprs }
- def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) => T, combOp: (T, T) => T): T = {
- if (this.isInstanceOf[A]) {
- val tv = seqOp(this.asInstanceOf[A], z)
- children.foldLeft(combOp(z, tv)) { (ov, tn) =>
- combOp(ov, tn.preOrderTraverseDepthFirst(z)(seqOp, combOp))
+ def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) => T, combOp: (T, T) => T)(implicit tag: ClassTag[A]): T = {
+
+ val clazz = tag.runtimeClass
+
+ this.getClass match {
+ case `clazz` => {
--- End diff --
we cannot use this patch, please remove this .
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Posted by bhlx3lyx7 <gi...@git.apache.org>.
Github user bhlx3lyx7 commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210831530
--- Diff: measure/src/main/scala/org/apache/griffin/measure/datasource/connector/DataConnectorFactory.scala ---
@@ -91,11 +91,12 @@ object DataConnectorFactory extends Loggable {
): KafkaStreamingDataConnector = {
val KeyType = "key.type"
val ValueType = "value.type"
- val config = dcParam.config
+ val config = dcParam.getConfig
val keyType = config.getOrElse(KeyType, "java.lang.String").toString
val valueType = config.getOrElse(ValueType, "java.lang.String").toString
- (getClassTag(keyType), getClassTag(valueType)) match {
- case (ClassTag(k: Class[String]), ClassTag(v: Class[String])) => {
+
+ (keyType, valueType) match {
+ case ("java.lang.String", "java.lang.String") => {
KafkaStreamingStringDataConnector(sparkSession, ssc, dcParam, tmstCache, streamingCacheClientOpt)
--- End diff --
I think match with literal string is too strong with limitation, why not match with the class type?
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Posted by grant-xuexu <gi...@git.apache.org>.
Github user grant-xuexu commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210918385
--- Diff: measure/src/main/scala/org/apache/griffin/measure/step/builder/dsl/expr/TreeNode.scala ---
@@ -18,28 +18,42 @@ under the License.
*/
package org.apache.griffin.measure.step.builder.dsl.expr
+import scala.reflect.ClassTag
+
trait TreeNode extends Serializable {
var children = Seq[TreeNode]()
def addChild(expr: TreeNode) = { children :+= expr }
def addChildren(exprs: Seq[TreeNode]) = { children ++= exprs }
- def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) => T, combOp: (T, T) => T): T = {
- if (this.isInstanceOf[A]) {
- val tv = seqOp(this.asInstanceOf[A], z)
- children.foldLeft(combOp(z, tv)) { (ov, tn) =>
- combOp(ov, tn.preOrderTraverseDepthFirst(z)(seqOp, combOp))
+ def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) => T, combOp: (T, T) => T)(implicit tag: ClassTag[A]): T = {
+
+ val clazz = tag.runtimeClass
+
+ this.getClass match {
+ case `clazz` => {
--- End diff --
@bhlx3lyx7 yea, you are correct. I misunderstood the logic here. However the issue is `if(this.instanceOf[A])` could already return true because of the type erasure at runtime.
There is a method called `isAssignableFrom` in Java Class[T], which could be used to check if a class is the subclass of another class.
so the logic could be changed into:
`
val clazz = tag.runtimeClass
if(clazz.isAssignableFrom(this.getClass)){
val tv = seqOp(this.asInstanceOf[A], z)
children.foldLeft(combOp(z, tv)) { (ov, tn) =>
combOp(ov, tn.preOrderTraverseDepthFirst(z)(seqOp, combOp))
}
}
else {
z
}
`
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Posted by grant-xuexu <gi...@git.apache.org>.
Github user grant-xuexu commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210923398
--- Diff: measure/src/main/scala/org/apache/griffin/measure/datasource/connector/DataConnectorFactory.scala ---
@@ -91,11 +91,12 @@ object DataConnectorFactory extends Loggable {
): KafkaStreamingDataConnector = {
val KeyType = "key.type"
val ValueType = "value.type"
- val config = dcParam.config
+ val config = dcParam.getConfig
val keyType = config.getOrElse(KeyType, "java.lang.String").toString
val valueType = config.getOrElse(ValueType, "java.lang.String").toString
- (getClassTag(keyType), getClassTag(valueType)) match {
- case (ClassTag(k: Class[String]), ClassTag(v: Class[String])) => {
+
+ (keyType, valueType) match {
+ case ("java.lang.String", "java.lang.String") => {
KafkaStreamingStringDataConnector(sparkSession, ssc, dcParam, tmstCache, streamingCacheClientOpt)
--- End diff --
the logic here seems assume the key type and value type are both String, or throw the exception.
And the type erasure makes `case (ClassTag(k: Class[String]), ClassTag(v: Class[String])) => {` already true
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Posted by bhlx3lyx7 <gi...@git.apache.org>.
Github user bhlx3lyx7 commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r211293186
--- Diff: measure/src/main/scala/org/apache/griffin/measure/datasource/connector/DataConnectorFactory.scala ---
@@ -91,11 +91,12 @@ object DataConnectorFactory extends Loggable {
): KafkaStreamingDataConnector = {
val KeyType = "key.type"
val ValueType = "value.type"
- val config = dcParam.config
+ val config = dcParam.getConfig
val keyType = config.getOrElse(KeyType, "java.lang.String").toString
val valueType = config.getOrElse(ValueType, "java.lang.String").toString
- (getClassTag(keyType), getClassTag(valueType)) match {
- case (ClassTag(k: Class[String]), ClassTag(v: Class[String])) => {
+
+ (keyType, valueType) match {
+ case ("java.lang.String", "java.lang.String") => {
KafkaStreamingStringDataConnector(sparkSession, ssc, dcParam, tmstCache, streamingCacheClientOpt)
--- End diff --
OK, I think you're right, it will be better to match with string here.
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Posted by bhlx3lyx7 <gi...@git.apache.org>.
Github user bhlx3lyx7 commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210831681
--- Diff: measure/src/main/scala/org/apache/griffin/measure/datasource/connector/DataConnector.scala ---
@@ -33,7 +33,7 @@ import org.apache.spark.sql.functions._
trait DataConnector extends Loggable with Serializable {
- @transient val sparkSession: SparkSession
+ val sparkSession: SparkSession
--- End diff --
SparkSession is Serializable, so it should works here.
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Posted by guoyuepeng <gi...@git.apache.org>.
Github user guoyuepeng commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210855056
--- Diff: measure/src/main/scala/org/apache/griffin/measure/step/builder/dsl/expr/TreeNode.scala ---
@@ -18,28 +18,42 @@ under the License.
*/
package org.apache.griffin.measure.step.builder.dsl.expr
+import scala.reflect.ClassTag
+
trait TreeNode extends Serializable {
var children = Seq[TreeNode]()
def addChild(expr: TreeNode) = { children :+= expr }
def addChildren(exprs: Seq[TreeNode]) = { children ++= exprs }
- def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) => T, combOp: (T, T) => T): T = {
- if (this.isInstanceOf[A]) {
- val tv = seqOp(this.asInstanceOf[A], z)
- children.foldLeft(combOp(z, tv)) { (ov, tn) =>
- combOp(ov, tn.preOrderTraverseDepthFirst(z)(seqOp, combOp))
+ def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) => T, combOp: (T, T) => T)(implicit tag: ClassTag[A]): T = {
+
+ val clazz = tag.runtimeClass
+
+ this.getClass match {
+ case `clazz` => {
--- End diff --
@bhlx3lyx7 @grant-xuexu we will append a test unit for this.
---
[GitHub] incubator-griffin issue #396: set the fields in configuration classes privat...
Posted by grant-xuexu <gi...@git.apache.org>.
Github user grant-xuexu commented on the issue:
https://github.com/apache/incubator-griffin/pull/396
"@transient" already is applied to all the child classes
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-griffin/pull/396
---
[GitHub] incubator-griffin issue #396: set the fields in configuration classes privat...
Posted by bhlx3lyx7 <gi...@git.apache.org>.
Github user bhlx3lyx7 commented on the issue:
https://github.com/apache/incubator-griffin/pull/396
I think we can merge this PR now.
---
[GitHub] incubator-griffin pull request #396: set the fields in configuration classes...
Posted by bhlx3lyx7 <gi...@git.apache.org>.
Github user bhlx3lyx7 commented on a diff in the pull request:
https://github.com/apache/incubator-griffin/pull/396#discussion_r210843748
--- Diff: measure/src/main/scala/org/apache/griffin/measure/step/builder/dsl/expr/TreeNode.scala ---
@@ -18,28 +18,42 @@ under the License.
*/
package org.apache.griffin.measure.step.builder.dsl.expr
+import scala.reflect.ClassTag
+
trait TreeNode extends Serializable {
var children = Seq[TreeNode]()
def addChild(expr: TreeNode) = { children :+= expr }
def addChildren(exprs: Seq[TreeNode]) = { children ++= exprs }
- def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) => T, combOp: (T, T) => T): T = {
- if (this.isInstanceOf[A]) {
- val tv = seqOp(this.asInstanceOf[A], z)
- children.foldLeft(combOp(z, tv)) { (ov, tn) =>
- combOp(ov, tn.preOrderTraverseDepthFirst(z)(seqOp, combOp))
+ def preOrderTraverseDepthFirst[T, A <: TreeNode](z: T)(seqOp: (A, T) => T, combOp: (T, T) => T)(implicit tag: ClassTag[A]): T = {
+
+ val clazz = tag.runtimeClass
+
+ this.getClass match {
+ case `clazz` => {
--- End diff --
The code means this.getClass and T must be the same class. But in the usage, it traverses among different types of tree nodes, their concrete classes can't be matched to their parent class, which is not what we want.
I doubt the modification of this file will destroy the process, I suggest you write a unit test for this.
---