Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 47 #123

Merged
merged 1 commit into from Oct 23, 2020
Merged

Issue 47 #123

merged 1 commit into from Oct 23, 2020

Conversation

gnodet
Copy link
Contributor

@gnodet gnodet commented Oct 21, 2020

No description provided.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 21, 2020

My main concern with the last commit is that we are adding the fourth (if I am counting correctly) thread on the client side. The threads are:

  • main
  • TerminalOutput.displayLoop
  • TerminalOutput.readInputLoop
  • This new one

Also the TerminalOutput owns an event queue. I wonder whether we could have just one queue and one thread hosting both display and dead daemon detection?

@gnodet
Copy link
Contributor Author

gnodet commented Oct 22, 2020

My main concern with the last commit is that we are adding the fourth (if I am counting correctly) thread on the client side. The threads are:

  • main
  • TerminalOutput.displayLoop
  • TerminalOutput.readInputLoop
  • This new one

Also the TerminalOutput owns an event queue. I wonder whether we could have just one queue and one thread hosting both display and dead daemon detection?

You're right. The problem is that the TerminalOutput is used in 2 scenarii: when using the client, and when using a parallel build without a daemon (using the mvns script). This makes things a bit complicated to rework in a clean way.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 22, 2020

@gnodet would you mind creating a separate PR for the last commit, so that we unblock the first three commits?

@famod
Copy link
Contributor

famod commented Oct 22, 2020

FWIW, if you guys could provide me with a native image for Windows that incorporates 00aed21 (loopback address), I could test it before it is being released.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 22, 2020

FWIW, if you guys could provide me with a native image for Windows that incorporates 00aed21 (loopback address), I could test it before it is being released.

That's highly welcome, thanks! A windows package will be available through the CI UI once @gnodet removes the last commit and the build will (hopefully) pass.

@famod
Copy link
Contributor

famod commented Oct 22, 2020

@ppalaga Thanks for the pointer to CI. I forked this repo and activated GH actions so that I don't have to setup GraalVM and native stuff locally. It might be worth adding this as an alternative way to README.

@famod
Copy link
Contributor

famod commented Oct 22, 2020

I think we should continue over there: #124

@ppalaga
Copy link
Contributor

ppalaga commented Oct 22, 2020

activated GH actions so that I don't have to setup GraalVM and native stuff locally.

What a trick! BTW, that's actually my strategy for Windows.

It might be worth adding this as an alternative way to README.

+1 a PR would be welcome.

@gnodet gnodet force-pushed the issue-47 branch 3 times, most recently from 00db8fd to d2a91b7 Compare October 22, 2020 15:43
@ppalaga
Copy link
Contributor

ppalaga commented Oct 22, 2020

The last iteration looks correct, but I'd like to think more about the performance impact and whether it can be done more effectively.

@ppalaga
Copy link
Contributor

ppalaga commented Oct 23, 2020

Merging as the perf impact does not seem to be perceivable, at least on my machine. I find the architectural concern still valid and I will file a follow up issue for that.

@ppalaga ppalaga merged commit a11b4c4 into apache:master Oct 23, 2020
@gnodet gnodet deleted the issue-47 branch October 26, 2020 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants