added a graceful shutdown method, added -spawnChild mode #10

ruesier · 2019-08-24T18:32:13Z

Should be backwards compatible with previous versions of package

Running with the spawnChild option makes Tika Server Robust to OOMs, Infinite Loops and Memory Leaks: https://cwiki.apache.org/confluence/display/tika/TikaJAXRS#TikaJAXRS-MakingTikaServerRobusttoOOMs,InfiniteLoopsandMemoryLeaks

Should be backwards compatible with previous versions

googlebot · 2019-08-24T18:32:15Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

ruesier · 2019-08-24T18:34:24Z

@googlebot I signed it!

googlebot · 2019-08-24T18:34:27Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

tbpg

Thanks!

tika/server.go

tbpg · 2019-08-27T19:09:26Z

tika/server.go

@@ -67,14 +77,35 @@ func NewServer(jar, port string) (*Server, error) {
 	return s, nil
 }

+// ChildMode sets up the server to use the -spawnChild option
+// must be called before starting the server
+func (s *Server) ChildMode(maxfiles, taskpulse, tasktimeout, pingpulse, pingtimeout int) error {


Please switch these args to camelCase. https://github.com/golang/go/wiki/CodeReviewComments#mixed-caps

tbpg · 2019-08-27T19:11:18Z

tika/server.go

+			case <-ctx.Done():
+			}
+	}()
+	select {


Could this all be a single select statement?

Your right, I'll change this, Was originally thinking that I was going to need two threads to handle the Signalling and error handling.

Actually no, went to implement this change and realized that s.cmd.Wait() runs synchronously. So s.cmd.Wait() can't be a case in a select statement, I need to wrap it inside it's own go routine to make it asynchronous with the context.Context.

Ah. That definitely makes sense. Mind adding a comment to Stop referencing Shutdown?

tbpg · 2019-08-27T19:11:37Z

tika/server.go

@@ -123,6 +154,32 @@ func (s *Server) Stop() error {
 	return nil
 }

+// Shutdown attempts to close the server gracefully before using SIGKILL,


Should we change the behavior of Stop and add a context.Context argument? If someone wants to kill immediately, they can pass a context.Context that is Done.

We can but that would make this update not backwards compatible. I added a Shutdown Method in order to not conflict with Stop. Also the golang documentation, on sending signals in the "os" package, warns that Sending Interrupt is not implemented in Windows. (https://golang.org/pkg/os/#Process.Signal). So currently Shutdown would not be advised in a Windows environment.

Yes, passing a context.Context that is Done would currently send a SIGINT signal followed immediately by a SIGKILL. That was the intended behavior of Shutdown.

tika/server.go

tbpg · 2019-08-27T19:17:29Z

tika/server.go

@@ -67,14 +77,35 @@ func NewServer(jar, port string) (*Server, error) {
 	return s, nil
 }

+// ChildMode sets up the server to use the -spawnChild option
+// must be called before starting the server
+func (s *Server) ChildMode(maxfiles, taskpulse, tasktimeout, pingpulse, pingtimeout int) error {


Are all of these arguments required? Will you update the function comment either way? (And handle missing args as needed.)

how about making the childOptions struct public (-> ChildOptions), then make the parameter to the ChildMode just a pointer to a ChildOptions.

Co-Authored-By: Tyler Bui-Palsulich <[email protected]>

tbpg

A few other small comments then good to go! Thank you!

tika/server.go

tbpg · 2019-08-30T14:34:44Z

tika/server.go

+
+func (co *ChildOptions) getArgs() []string {
+	if co == nil {
+		return []string{}


In general, should avoid making a distinction between nil and an empty slice. So, it's most common to just return nil.

tika/server.go

tbpg · 2019-08-30T14:37:14Z

tika/server.go

+			case <-ctx.Done():
+			}
+	}()
+	select {


Ah. That definitely makes sense. Mind adding a comment to Stop referencing Shutdown?

Co-Authored-By: Tyler Bui-Palsulich <[email protected]>

ruesier · 2019-09-01T22:30:57Z

alright, enacted the changed you recommended, let me know if there is anything else I could do to pass the checks

tbpg · 2019-09-23T12:43:33Z

@ruesier So sorry for the delay! Was on vacation for a week and am just getting around to this.

I fixed the issue causing the tests to fail. So, if you merge the latest changes from master, it should all work.

Looks like you may have used two emails for the commits in this PR, and one of them (@calld) doesn't have a CLA on file. Would you mind changing the author emails to be consistent?

googlebot · 2019-09-23T21:33:09Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

calld · 2019-09-23T21:39:20Z

alright updated commit history to correctly identify me so that googlebot recognizes that I have a valid CLA. and synced my fork with current version of repo. I believe it is ready to go

tbpg · 2019-09-23T21:41:08Z

Thank you!

added a graceful shutdown method, added -spawnChild mode

01b3ebd

Should be backwards compatible with previous versions

tbpg requested changes Aug 27, 2019

View reviewed changes

ruesier and others added 3 commits August 27, 2019 22:36

Update tika/server.go

d4aa3b2

Co-Authored-By: Tyler Bui-Palsulich <[email protected]>

Update tika/server.go

ab045d7

Co-Authored-By: Tyler Bui-Palsulich <[email protected]>

Made ChildOptions public to simplify ChildMode

17d699d

tbpg requested changes Aug 30, 2019

View reviewed changes

ruesier and others added 6 commits September 1, 2019 14:33

Update tika/server.go

b0566f1

Co-Authored-By: Tyler Bui-Palsulich <[email protected]>

Update tika/server.go

8871934

Co-Authored-By: Tyler Bui-Palsulich <[email protected]>

Update tika/server.go

90ea584

Co-Authored-By: Tyler Bui-Palsulich <[email protected]>

Update tika/server.go

ecdad17

Co-Authored-By: Tyler Bui-Palsulich <[email protected]>

Update tika/server.go

9a464e9

Co-Authored-By: Tyler Bui-Palsulich <[email protected]>

code clean up and updated comments

1e572d0

travis: update versions (google#11)

e44ed81

Merge remote-tracking branch 'upstream/master'

7a93ba3

ruesier force-pushed the master branch from a0e57a0 to 7a93ba3 Compare September 23, 2019 21:32

tbpg approved these changes Sep 23, 2019

View reviewed changes

tbpg merged commit 92d0506 into google:master Sep 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added a graceful shutdown method, added -spawnChild mode #10

added a graceful shutdown method, added -spawnChild mode #10

ruesier commented Aug 24, 2019

googlebot commented Aug 24, 2019

ruesier commented Aug 24, 2019

googlebot commented Aug 24, 2019

tbpg left a comment

tbpg Aug 27, 2019

ruesier Aug 28, 2019

tbpg Aug 27, 2019

ruesier Aug 28, 2019

ruesier Aug 28, 2019

tbpg Aug 30, 2019

tbpg Aug 27, 2019

ruesier Aug 28, 2019

tbpg Aug 27, 2019

ruesier Aug 28, 2019

tbpg left a comment

tbpg Aug 30, 2019

tbpg Aug 30, 2019

ruesier commented Sep 1, 2019

tbpg commented Sep 23, 2019

googlebot commented Sep 23, 2019

calld commented Sep 23, 2019

tbpg commented Sep 23, 2019

added a graceful shutdown method, added -spawnChild mode #10

added a graceful shutdown method, added -spawnChild mode #10

Conversation

ruesier commented Aug 24, 2019

googlebot commented Aug 24, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

ruesier commented Aug 24, 2019

googlebot commented Aug 24, 2019

tbpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruesier commented Sep 1, 2019

tbpg commented Sep 23, 2019

googlebot commented Sep 23, 2019

calld commented Sep 23, 2019

tbpg commented Sep 23, 2019