nickpan47 commented on code in PR #1628:
URL: https://github.com/apache/samza/pull/1628#discussion_r983075086
##########
gradle.properties:
##########
@@ -16,10 +16,14 @@
# under the License.
group=org.apache.samza
version=1.7.0-SNAPSHOT
+# These 2 are ones that you can override using properties, like:
+# -PscalaSuffix=2.12 -PyarnVersion=2.10.1
scalaSuffix=2.12
+yarnVersion=2.10.1
+yarn3Version=3.3.4
Review Comment:
Add a comment to state that this is to support JDK11 runtime.
##########
samza-log4j2/src/main/java/org/apache/samza/logging/log4j2/StreamAppender.java:
##########
@@ -73,7 +73,7 @@ public class StreamAppender extends AbstractAppender {
private final BlockingQueue<EncodedLogEvent> logQueue = new
LinkedBlockingQueue<>(DEFAULT_QUEUE_SIZE);
private SystemStream systemStream = null;
- private SystemProducer systemProducer = null;
+ protected SystemProducer systemProducer = null;
Review Comment:
Why are we changing this to "protected"? I would recommend keeping this PR
just for JDK11 changes. If this is needed for other reasons, please open a
separate PR.
P.S. it seems PR #1627 has already made the change.
##########
samza-shell/src/main/bash/run-class.sh:
##########
@@ -153,7 +153,7 @@ fi
[[ $JAVA_OPTS != *-Xmx* ]] && JAVA_OPTS="$JAVA_OPTS -Xmx768M"
# Check if the GC related flags are specified. If not - add the respective
flags to JVM_OPTS.
-[[ $JAVA_OPTS != *PrintGCDateStamps* && $JAVA_OPTS != *-Xloggc* ]] &&
JAVA_OPTS="$JAVA_OPTS -XX:+PrintGCDateStamps -Xloggc:$SAMZA_LOG_DIR/gc.log"
+[[ $JAVA_OPTS != *-Xloggc* ]] && JAVA_OPTS="$JAVA_OPTS
-Xloggc:$SAMZA_LOG_DIR/gc.log"
Review Comment:
Can we make it conditional? i.e. if JAVA_HOME is 8, use this, if JAVA_HOME
is 11, don't use this?
##########
README.md:
##########
@@ -28,9 +28,12 @@ After the bootstrap script has completed, the regular
gradlew instructions below
#### Scala and YARN
-Samza builds with [Scala](http://www.scala-lang.org/) 2.11 or 2.12 and
[YARN](http://hadoop.apache.org/docs/current/hadoop-yarn/hadoop-yarn-site/YARN.html)
2.6.1, by default. Use the -PscalaSuffix switches to change Scala versions.
Samza supports building Scala with 2.11 and 2.12.
+Samza builds with [Scala](http://www.scala-lang.org/) 2.11 or 2.12 and
[YARN](http://hadoop.apache.org/docs/current/hadoop-yarn/hadoop-yarn-site/YARN.html)
2.10.1 or 3.3.4. Scala 2.12 and Yarn 2.10.1 are used by default. Use the
-PscalaSuffix and -PyarnVersion switches to change Scala or Yarn versions.
Samza supports building Scala with 2.11 and 2.12.
Review Comment:
I think that we should still keep the original description, w/ default to
2.10.1. The separate build commands below are also not needed.
We may want to add a section separately regarding to JDK11 runtime support,
i.e. stating that Samza currently is still built w/ Java8 but support Java11
runtime, w/ samza-yarn3 built on YARN 3.3.4.
##########
samza-yarn/src/test/java/org/apache/samza/job/yarn/TestYarnFaultDomainManager.java:
##########
@@ -60,22 +60,22 @@ public class TestYarnFaultDomainManager {
private final NodeReport nodeReport1 = createNodeReport(hostName1, 1,
NodeState.RUNNING, "httpAddress1",
rackName1, 1, 1, 2, 1, 2,
- "", 60L, null);
+ "", 60L);
Review Comment:
Is this change for YARN 3.3.4? I would suggest leave it out since we are not
changing samza-yarn module for YARN 3.3.4.
##########
gradle.properties:
##########
@@ -16,10 +16,14 @@
# under the License.
group=org.apache.samza
version=1.7.0-SNAPSHOT
+# These 2 are ones that you can override using properties, like:
+# -PscalaSuffix=2.12 -PyarnVersion=2.10.1
scalaSuffix=2.12
+yarnVersion=2.10.1
+yarn3Version=3.3.4
# after changing this value, run `$ ./gradlew wrapper` and commit the
resulting changed files
-gradleVersion=5.2.1
+gradleVersion=6.9.2
Review Comment:
Is this strictly needed for JDK11 runtime support? @mynameborat can you
comment on that?
##########
samza-yarn/src/test/java/org/apache/samza/job/yarn/TestYarnFaultDomainManager.java:
##########
@@ -182,9 +182,17 @@ private void assertFaultDomainEquals(FaultDomain
faultDomain1, FaultDomain fault
private NodeReport createNodeReport(String host, int port, NodeState
nodeState, String httpAddress, String rackName,
int memoryUsed, int vcoresUsed, int
totalMemory, int totalVcores, int numContainers,
- String healthReport, long
lastHealthReportTime, Set<String> nodeLabels) {
- return NodeReport.newInstance(NodeId.newInstance(host, port), nodeState,
httpAddress, rackName,
- Resource.newInstance(memoryUsed, vcoresUsed),
Resource.newInstance(totalMemory, totalVcores), numContainers,
- healthReport, lastHealthReportTime, nodeLabels);
+ String healthReport, long
lastHealthReportTime) {
+ return NodeReport.newInstance(
Review Comment:
Same comment here: is this change for YARN 3.3.4? If not, let's leave
changes not related to JDK11 runtime support out of this PR.
##########
README.md:
##########
@@ -28,9 +28,12 @@ After the bootstrap script has completed, the regular
gradlew instructions below
#### Scala and YARN
-Samza builds with [Scala](http://www.scala-lang.org/) 2.11 or 2.12 and
[YARN](http://hadoop.apache.org/docs/current/hadoop-yarn/hadoop-yarn-site/YARN.html)
2.6.1, by default. Use the -PscalaSuffix switches to change Scala versions.
Samza supports building Scala with 2.11 and 2.12.
+Samza builds with [Scala](http://www.scala-lang.org/) 2.11 or 2.12 and
[YARN](http://hadoop.apache.org/docs/current/hadoop-yarn/hadoop-yarn-site/YARN.html)
2.10.1 or 3.3.4. Scala 2.12 and Yarn 2.10.1 are used by default. Use the
-PscalaSuffix and -PyarnVersion switches to change Scala or Yarn versions.
Samza supports building Scala with 2.11 and 2.12.
Review Comment:
Also, please list the modules that support jdk11 runtime in the above
section (i.e. not samza-yarn, samza-shell, samza-test, and samza-hdfs, since
those modules are still on YARN 2.10.1).
##########
README.md:
##########
@@ -28,9 +28,12 @@ After the bootstrap script has completed, the regular
gradlew instructions below
#### Scala and YARN
-Samza builds with [Scala](http://www.scala-lang.org/) 2.11 or 2.12 and
[YARN](http://hadoop.apache.org/docs/current/hadoop-yarn/hadoop-yarn-site/YARN.html)
2.6.1, by default. Use the -PscalaSuffix switches to change Scala versions.
Samza supports building Scala with 2.11 and 2.12.
+Samza builds with [Scala](http://www.scala-lang.org/) 2.11 or 2.12 and
[YARN](http://hadoop.apache.org/docs/current/hadoop-yarn/hadoop-yarn-site/YARN.html)
2.10.1 or 3.3.4. Scala 2.12 and Yarn 2.10.1 are used by default. Use the
-PscalaSuffix and -PyarnVersion switches to change Scala or Yarn versions.
Samza supports building Scala with 2.11 and 2.12.
- ./gradlew -PscalaSuffix=2.11 clean build
+ ./gradlew -PscalaSuffix=2.12 -PyarnVersion=2.10.1 clean build
Review Comment:
The build command should still stay the same.
##########
samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaYarnAppMasterLifecycle.scala:
##########
@@ -78,6 +78,8 @@ class TestSamzaYarnAppMasterLifecycle {
override def setSchedulerResourceTypes(types:
java.util.EnumSet[SchedulerResourceTypes]): Unit = {}
override def getSchedulerResourceTypes:
java.util.EnumSet[SchedulerResourceTypes] = null
+ //override def getResourceProfiles():
java.util.Map[String,org.apache.hadoop.yarn.api.records.Resource] = ???
Review Comment:
Please remove the commented lines here.
##########
samza-log4j2/src/main/java/org/apache/samza/logging/log4j2/StreamAppender.java:
##########
@@ -416,7 +416,7 @@ private void startTransferThread() {
* Helper method to send a serialized log-event to the systemProducer, and
increment respective methods.
* @param logQueueEntry the serialized log-event to be sent to the
systemProducer
*/
- private void sendEventToSystemProducer(EncodedLogEvent logQueueEntry) {
+ protected void sendEventToSystemProducer(EncodedLogEvent logQueueEntry) {
Review Comment:
Same here. This is in PR#1627. Let's keep this PR only on JDK11 runtime
support.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
