You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "beliefer (via GitHub)" <gi...@apache.org> on 2023/12/09 04:04:21 UTC
[PR] [WIP][Used for discussion and decision-making] Improve the java text block with java15 feature. [spark]
beliefer opened a new pull request, #44270:
URL: https://github.com/apache/spark/pull/44270
### What changes were proposed in this pull request?
Now, Spark upgraded to Java17 which contains many new feature. The text block is a feature released in Java15.
When using text blocks, the code looks more concise.
This PR is a initial one used for discussion and decision-making.
Shall we improve and simplify the long string with java15 text block?
### Why are the changes needed?
This PR improves the java text block with java15 feature.
### Does this PR introduce _any_ user-facing change?
'No'.
### How was this patch tested?
GA.
### Was this patch authored or co-authored using generative AI tooling?
'No'.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][Used for discussion and decision-making] Improve the java text block with java15 feature. [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44270:
URL: https://github.com/apache/spark/pull/44270#issuecomment-1848217893
@dongjoon-hyun @srowen @LuciferYang What are your suggestions?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][Used for discussion and decision-making] Improve the java text block with java15 feature. [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on PR #44270:
URL: https://github.com/apache/spark/pull/44270#issuecomment-1849418258
fine to me, I think we should push it forward. @beliefer
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [WIP][Used for discussion and decision-making] Improve the java text block with java15 feature. [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44270:
URL: https://github.com/apache/spark/pull/44270#issuecomment-1848224659
cc @MaxGekk
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46363][CORE][SQL] Improve the java text block with java15 feature. [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer closed pull request #44270: [SPARK-46363][CORE][SQL] Improve the java text block with java15 feature.
URL: https://github.com/apache/spark/pull/44270
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46363][CORE][SQL] Improve the java text block with java15 feature. [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on PR #44270:
URL: https://github.com/apache/spark/pull/44270#issuecomment-1853827509
Based on the suggestion from @cloud-fan, I close this PR. I will reopen this PR if need.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46363][CORE][SQL] Improve the java text block with java15 feature. [spark]
Posted by "beliefer (via GitHub)" <gi...@apache.org>.
beliefer commented on code in PR #44270:
URL: https://github.com/apache/spark/pull/44270#discussion_r1423891063
##########
common/network-common/src/main/java/org/apache/spark/network/server/TransportChannelHandler.java:
##########
@@ -162,10 +162,11 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
if (e.state() == IdleState.ALL_IDLE && isActuallyOverdue) {
if (responseHandler.hasOutstandingRequests()) {
String address = getRemoteAddress(ctx.channel());
- logger.error("Connection to {} has been quiet for {} ms while there are outstanding " +
- "requests. Assuming connection is dead; please adjust" +
- " spark.{}.io.connectionTimeout if this is wrong.",
- address, requestTimeoutNs / 1000 / 1000, transportContext.getConf().getModuleName());
+ logger.error("""
+ Connection to {} has been quiet for {} ms while there are outstanding requests. \
Review Comment:
Yeah. It's the format for logger. Let's revert it.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46363][CORE][SQL] Improve the java text block with java15 feature. [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #44270:
URL: https://github.com/apache/spark/pull/44270#discussion_r1424841874
##########
sql/hive/src/test/java/org/apache/spark/sql/hive/JavaMetastoreDataSourcesSuite.java:
##########
@@ -67,7 +67,8 @@ public void setUp() throws IOException {
List<String> jsonObjects = new ArrayList<>(10);
for (int i = 0; i < 10; i++) {
- jsonObjects.add("{\"a\":" + i + ", \"b\":\"str" + i + "\"}");
+ jsonObjects.add("""
+ {"a":%d, "b":"str%d"}""".formatted(i, i));
Review Comment:
Indentation
##########
sql/hive/src/test/java/org/apache/spark/sql/hive/JavaDataFrameSuite.java:
##########
@@ -46,7 +46,8 @@ public void setUp() throws IOException {
hc = TestHive$.MODULE$;
List<String> jsonObjects = new ArrayList<>(10);
for (int i = 0; i < 10; i++) {
- jsonObjects.add("{\"key\":" + i + ", \"value\":\"str" + i + "\"}");
+ jsonObjects.add("""
+ {"key":%d, "value":"str%d"}""".formatted(i, i));
Review Comment:
Indentation
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-46363][CORE][SQL] Improve the java text block with java15 feature. [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #44270:
URL: https://github.com/apache/spark/pull/44270#discussion_r1423861669
##########
common/network-common/src/main/java/org/apache/spark/network/TransportContext.java:
##########
@@ -272,8 +272,9 @@ private SSLFactory createSslFactory() {
conf.sslRpctrustStoreReloadIntervalMs())
.build();
} else {
- logger.error("RPC SSL encryption enabled but keys not found!" +
- "Please ensure the configured keys are present.");
+ logger.error("""
+ RPC SSL encryption enabled but keys not found!\
Review Comment:
Indentation?
##########
common/network-common/src/main/java/org/apache/spark/network/server/TransportChannelHandler.java:
##########
@@ -162,10 +162,11 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc
if (e.state() == IdleState.ALL_IDLE && isActuallyOverdue) {
if (responseHandler.hasOutstandingRequests()) {
String address = getRemoteAddress(ctx.channel());
- logger.error("Connection to {} has been quiet for {} ms while there are outstanding " +
- "requests. Assuming connection is dead; please adjust" +
- " spark.{}.io.connectionTimeout if this is wrong.",
- address, requestTimeoutNs / 1000 / 1000, transportContext.getConf().getModuleName());
+ logger.error("""
+ Connection to {} has been quiet for {} ms while there are outstanding requests. \
Review Comment:
`{}` will not be replaced by parameters in the `formatted`, right?
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -281,14 +284,14 @@ AppShufflePartitionInfo getOrCreateAppShufflePartitionInfo(
return newAppShufflePartitionInfo(appShuffleInfo, shuffleId, shuffleMergeId, reduceId,
dataFile, indexFile, metaFile);
} catch (IOException e) {
- logger.error("{} attempt {} shuffle {} shuffleMerge {}: cannot create merged shuffle " +
- "partition with data file {}, index file {}, and meta file {}", appShuffleInfo.appId,
- appShuffleInfo.attemptId, shuffleId, shuffleMergeId, dataFile.getAbsolutePath(),
- indexFile.getAbsolutePath(), metaFile.getAbsolutePath());
- throw new RuntimeException(
- String.format("Cannot initialize merged shuffle partition for appId %s shuffleId %s "
- + "shuffleMergeId %s reduceId %s", appShuffleInfo.appId, shuffleId, shuffleMergeId,
- reduceId), e);
+ logger.error("""
Review Comment:
ditto
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -247,9 +248,11 @@ AppShufflePartitionInfo getOrCreateAppShufflePartitionInfo(
AppAttemptShuffleMergeId currrentAppAttemptShuffleMergeId =
new AppAttemptShuffleMergeId(appShuffleInfo.appId, appShuffleInfo.attemptId,
shuffleId, latestShuffleMergeId);
- logger.info("{}: creating a new shuffle merge metadata since received " +
- "shuffleMergeId {} is higher than latest shuffleMergeId {}",
- currrentAppAttemptShuffleMergeId, shuffleMergeId, latestShuffleMergeId);
+ logger.info("""
Review Comment:
ditto
##########
common/network-shuffle/src/main/java/org/apache/spark/network/shuffle/RemoteBlockPushResolver.java:
##########
@@ -228,9 +228,10 @@ AppShufflePartitionInfo getOrCreateAppShufflePartitionInfo(
AppShuffleMergePartitionsInfo shufflePartitionsWithMergeId =
shuffles.compute(shuffleId, (id, mergePartitionsInfo) -> {
if (mergePartitionsInfo == null) {
- logger.info("{} attempt {} shuffle {} shuffleMerge {}: creating a new shuffle " +
- "merge metadata", appShuffleInfo.appId, appShuffleInfo.attemptId, shuffleId,
- shuffleMergeId);
+ logger.info("""
Review Comment:
ditto
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org