Skip to content

Conversation

@CalvinKirs
Copy link
Member

No description provided.

Comment on lines 61 to 63
if(StringUtils.isNotBlank(System.getProperty("zookeeper.quorum"))){
return System.getProperty("zookeeper.quorum");
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid modifying too much, do this temporarily and the dev version is OK.

@CalvinKirs
Copy link
Member Author

@kezhenxu94 hi, can you help me review?thx

@CalvinKirs CalvinKirs added this to the 1.3.9 milestone Oct 11, 2021
@kezhenxu94
Copy link
Member

@kezhenxu94 hi, can you help me review?thx

Sure. Will take a look soon.

@CalvinKirs CalvinKirs added the enhancement New feature or request label Oct 11, 2021
@kezhenxu94
Copy link
Member

@CalvinKirs can we just cherry pick the related commits into the target branch?

@kezhenxu94
Copy link
Member

Standalone server is mostly for convenient development, so I don't know how much it will benefit to cherry pick this to the 1.3.9 release, I think developers often develop on dev branch, right?

@CalvinKirs
Copy link
Member Author

@CalvinKirs can we just cherry pick the related commits into the target branch?

It’s very difficult, our branches are quite different 😭

@CalvinKirs
Copy link
Member Author

CalvinKirs commented Oct 11, 2021

Standalone server is mostly for convenient development, so I don't know how much it will benefit to cherry pick this to the 1.3.9 release, I think developers often develop on dev branch, right?

I totally agree with you. but the main purpose of doing this is to allow some new users to start easily and quickly. @dailidong

@CalvinKirs CalvinKirs added the discussion discussion label Oct 11, 2021
@davidzollo
Copy link
Contributor

Standalone server is mostly for convenient development, so I don't know how much it will benefit to cherry pick this to the 1.3.9 release, I think developers often develop on dev branch, right?

I totally agree with you. but the main purpose of doing this is to allow some new users to start easily and quickly. @dailidong

Now there are many deployment steps for DolphinScheduler, especially for new users, what they want to feel most is to quickly experience whether this can meet his needs and how to quickly experience the functions of DolphinScheduler.

Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in terms of standalone server functionalities, I can see this error logs when running a simple shell task, might not be related to this but might be worthy to take a look.

2021-10-12 20:04:02.210 ERROR 76462 --- [ool-13-thread-2] o.a.d.remote.handler.NettyServerHandler  : process msg Command [type=TASK_EXECUTE_REQUEST, opaque=3, bodyLen=1510] error

java.lang.NullPointerException: null
	at org.apache.dolphinscheduler.server.worker.processor.TaskExecuteProcessor.getTaskLogPath(TaskExecuteProcessor.java:175) ~[classes/:na]
	at org.apache.dolphinscheduler.server.worker.processor.TaskExecuteProcessor.process(TaskExecuteProcessor.java:133) ~[classes/:na]
	at org.apache.dolphinscheduler.remote.handler.NettyServerHandler$1.run(NettyServerHandler.java:134) ~[classes/:na]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511) [na:1.8.0_292]
	at java.util.concurrent.FutureTask.run(FutureTask.java:266) [na:1.8.0_292]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [na:1.8.0_292]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [na:1.8.0_292]
	at java.lang.Thread.run(Thread.java:748) [na:1.8.0_292]

private int maxWaitTime;

public String getServerList() {
if(StringUtils.isNotBlank(System.getProperty("zookeeper.quorum"))){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it's not formatted

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx your remind, done

Comment on lines +70 to +72
private static void startAlertServer() {
AlertServer.getInstance().start();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you cherry-pick this part from the latest dev branch and uncomment line 61 to enable alert server?

Suggested change
private static void startAlertServer() {
AlertServer.getInstance().start();
}
private static void startAlertServer() {
final Path alertPluginPath = Paths.get(
StandaloneServer.class.getProtectionDomain().getCodeSource().getLocation().getPath(),
"../../../dolphinscheduler-alert-plugin/dolphinscheduler-alert-email/pom.xml"
).toAbsolutePath();
if (Files.exists(alertPluginPath)) {
System.setProperty("alert.plugin.binding", alertPluginPath.toString());
System.setProperty("alert.plugin.dir", "");
}
AlertServer.getInstance().start();
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the settings are totally different between this branch and dev branch?

I can see

alert.type=EMAIL
plugin.dir=/Users/xx/your/path/to/plugin/dir

vs

alert.plugin.binding=
alert.plugin.dir=

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, an Email Alert is that must be loaded. An email will be loaded automatically when AlertServer is started

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we don’t need to set up email

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link
Member

@kezhenxu94 kezhenxu94 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@lenboo lenboo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@lenboo lenboo merged commit 7223469 into apache:1.3.9-prepare Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

discussion discussion enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants