Copilot commented on code in PR #1640:
URL: https://github.com/apache/auron/pull/1640#discussion_r2554634562
##########
spark-extension-shims-spark/src/main/java/org/apache/spark/sql/auron/ValidateSparkPlanInjector.java:
##########
@@ -31,19 +31,27 @@
public class ValidateSparkPlanInjector {
private static final Logger logger =
LoggerFactory.getLogger(ValidateSparkPlanInjector.class);
+ private static volatile boolean classNotFound = false;
private static boolean injected = false;
public static synchronized void inject() {
if (injected) {
logger.warn("ValidateSparkPlan already injected, skipping.");
return;
}
+ if (classNotFound) {
Review Comment:
Consider adding a log message when skipping injection due to `classNotFound`
being true. This would provide better observability and debugging information,
similar to the existing log message at line 39 for the `injected` case. For
example: `logger.debug("ValidateSparkPlan class not found, skipping
injection.");`
```suggestion
if (classNotFound) {
logger.debug("ValidateSparkPlan class not found, skipping
injection.");
```
##########
spark-extension-shims-spark/src/main/java/org/apache/spark/sql/auron/ValidateSparkPlanInjector.java:
##########
@@ -31,19 +31,27 @@
public class ValidateSparkPlanInjector {
private static final Logger logger =
LoggerFactory.getLogger(ValidateSparkPlanInjector.class);
+ private static volatile boolean classNotFound = false;
private static boolean injected = false;
Review Comment:
The `injected` field should also be declared as `volatile` for thread
safety, similar to `classNotFound`. While the method is synchronized, the read
of `injected` at line 38 occurs without synchronization from other potential
readers. This ensures proper visibility of the flag across threads.
```suggestion
private static volatile boolean injected = false;
```
--
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]
