Adding support for setting java system properties (go-tika #15) #16

dmnyu · 2020-02-26T14:20:39Z

This pull requests

Adds a javaprops map to Server
Adds a AddJavaProp function to add key-value pairs to a Servers javaprops map
Modifies the Start() function to check if the javaprops map is not empty, if so it formats key value pairs into system property setters to the java command.
Adds a test to check that kv pairs are being added to javaprops map

… Start function to check for added props, wrote test.

tbpg · 2020-02-26T16:41:44Z

tika/server.go

+	port      string
+	cmd       *exec.Cmd
+	child     *ChildOptions
+	javaprops map[string]string


Can we export this variable and have people set it directly? Then we won't need the AddJavaProp function, which will be nicer when setting multiple properties.

Suggested change

javaprops map[string]string

JavaProps map[string]string

dmnyu · 2020-02-26T17:59:17Z

That makes a lot of sense. I'll make the changes.

…dating test.

tbpg

Thanks, again, for doing this!

Heads up, I'll be slow to respond until March 16.

tbpg · 2020-03-05T09:09:36Z

tika/server.go

@@ -41,7 +41,7 @@ type Server struct {
 	port      string
 	cmd       *exec.Cmd
 	child     *ChildOptions
-	javaprops map[string]string
+	Javaprops map[string]string


Probably JavaProps? Also, please add a documentation comment for this field. What is it for, how is it passed to the server, etc.

tbpg · 2020-03-05T09:10:06Z

tika/server.go

 	s.url = u.String()
+
+	s.Javaprops = make(map[string]string)


Mind moving this above to the declaration of s? Does it need to be pre-initialized?

tbpg · 2020-03-05T09:10:45Z

tika/server.go

-	if len(s.javaprops) > 0 {
-		for i := range s.javaprops {
-			props = fmt.Sprintf("-D%s=%s ", i, s.javaprops[i])
+	if len(s.Javaprops) > 0 {


range works well on an empty map. So, we can skip this check.

tbpg · 2020-03-05T09:13:08Z

tika/server_test.go


 	want := 1
-	got := len(s.javaprops)
+	got := len(s.Javaprops)


Is there a way we can test that the arguments are correctly added to the java command? Would hopefully catch the overwriting props every time bug.

tbpg · 2020-03-05T09:16:45Z

tika/server.go

-			props = fmt.Sprintf("-D%s=%s ", i, s.javaprops[i])
+	if len(s.Javaprops) > 0 {
+		for i := range s.Javaprops {
+			props = fmt.Sprintf("-D%s=%s ", i, s.Javaprops[i])


This overwrites props on every iteration. Perhaps something like this?

props := []string{} for k, v := range s.JavaProps { props = append(props, fmt.Sprintf("-D%s=%s", k, v)) } propsArgs := strings.Join(props, " ")

(Also gets the key and value from the range, rather than just the key.)

tbpg

Sorry for the slow review (just getting back from OOO). Thanks for working on this. :)

tbpg · 2020-03-19T20:41:58Z

tika/server.go


-	cmd := command("java", append([]string{props, "-jar", s.jar, "-p", s.port}, s.child.args()...)...)
+	cmd := command("java", append([]string{propsArgs, "-jar", s.jar, "-p", s.port}, s.child.args()...)...)


I think all of the props should be passed as separate args? Probably need another append:

Suggested change

cmd := command("java", append([]string{propsArgs, "-jar", s.jar, "-p", s.port}, s.child.args()...)...)

cmd := command("java", append(append(props, []string{propsArgs, "-jar", s.jar, "-p", s.port}, s.child.args()...)...)...)

tbpg · 2020-03-19T20:42:33Z

tika/server.go

-		port: port,
+		jar:       jar,
+		port:      port,
+		JavaProps: javaProps,


Suggested change

JavaProps: javaProps,

JavaProps: map[string]string{},

dmnyu · 2020-03-23T17:46:35Z

Thanks Tyler,

I made all of the changes in my local repo and just need to update the comments and documentation. It probably goes without saying how crazy things are atm.

Don

… Start function to check for added props, wrote test.

…avaprops # Conflicts: # tika/server.go # tika/server_test.go

tbpg · 2020-03-23T17:51:54Z

Sounds good -- totally understand. No rush.

tbpg · 2020-03-24T12:16:21Z

tika/server.go


-	cmd := command("java", append([]string{propsArgs, "-jar", s.jar, "-p", s.port}, s.child.args()...)...)
+	cmd := command("java", append([]string{strings.Join(props, " "), "-jar", s.jar, "-p", s.port}, s.child.args()...)...)


Sorry, I probably wasn't clear. I don't think we should have spaces in the argument. Each argument should be a different element of this args slice.

Something like this, but we could do it all in one line. A little odd to have an append with different values on the right and left.

Suggested change

cmd := command("java", append([]string{strings.Join(props, " "), "-jar", s.jar, "-p", s.port}, s.child.args()...)...)

args := append(props, "-jar", s.jar, "-p", s.port)

args = append(args, s.child.args()...

cmd := command("java", args...)

tbpg · 2020-03-25T19:11:41Z

Merging and will send another PR for a few style changes.

Thank you!

dmnyu · 2020-03-26T14:49:08Z

Tyler, Thanks for all the help. this was the first PR I've done for a Go project.

tbpg · 2020-03-26T14:54:17Z

Thank you for the contribution! And welcome! 🎉

Added javapops map Server struct, AddJavaProps function, and modified…

ffec9b3

… Start function to check for added props, wrote test.

tbpg requested changes Feb 26, 2020

View reviewed changes

Exporting javaprops, initializing props map in NewServer function, up…

98eae9e

…dating test.

tbpg requested changes Mar 5, 2020

View reviewed changes

Updating Server methods and tests

79099ae

tbpg requested changes Mar 19, 2020

View reviewed changes

dmnyu added 2 commits March 23, 2020 13:47

Added javapops map Server struct, AddJavaProps function, and modified…

39cc252

… Start function to check for added props, wrote test.

Merge branch 'javaprops' of https://github.com/nyudlts/go-tika into j…

77e716c

…avaprops # Conflicts: # tika/server.go # tika/server_test.go

dmnyu added 2 commits March 23, 2020 13:56

Changing accidentle modification

38175ee

moving props.join function into cmd builder, annotating server struct.

e7abc66

tbpg reviewed Mar 24, 2020

View reviewed changes

Modified formatting of jvm system properties, reformattied cmd builder.

24f340d

tbpg approved these changes Mar 25, 2020

View reviewed changes

tbpg merged commit 41cad59 into google:master Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for setting java system properties (go-tika #15) #16

Adding support for setting java system properties (go-tika #15) #16

dmnyu commented Feb 26, 2020

tbpg Feb 26, 2020

dmnyu commented Feb 26, 2020

tbpg left a comment

tbpg Mar 5, 2020

tbpg Mar 5, 2020

tbpg Mar 5, 2020

tbpg Mar 5, 2020

tbpg Mar 5, 2020

tbpg left a comment

tbpg Mar 19, 2020

tbpg Mar 19, 2020

dmnyu commented Mar 23, 2020

tbpg commented Mar 23, 2020

tbpg Mar 24, 2020

tbpg commented Mar 25, 2020

dmnyu commented Mar 26, 2020

tbpg commented Mar 26, 2020


		cmd := command("java", append([]string{props, "-jar", s.jar, "-p", s.port}, s.child.args()...)...)
		cmd := command("java", append([]string{propsArgs, "-jar", s.jar, "-p", s.port}, s.child.args()...)...)

	cmd := command("java", append([]string{propsArgs, "-jar", s.jar, "-p", s.port}, s.child.args()...)...)
	cmd := command("java", append(append(props, []string{propsArgs, "-jar", s.jar, "-p", s.port}, s.child.args()...)...)...)


		cmd := command("java", append([]string{propsArgs, "-jar", s.jar, "-p", s.port}, s.child.args()...)...)
		cmd := command("java", append([]string{strings.Join(props, " "), "-jar", s.jar, "-p", s.port}, s.child.args()...)...)

-	cmd := command("java", append([]string{strings.Join(props, " "), "-jar", s.jar, "-p", s.port}, s.child.args()...)...)
+	args := append(props, "-jar", s.jar, "-p", s.port)
+	args = append(args, s.child.args()...
+	cmd := command("java", args...)

Adding support for setting java system properties (go-tika #15) #16

Adding support for setting java system properties (go-tika #15) #16

Conversation

dmnyu commented Feb 26, 2020

Choose a reason for hiding this comment

dmnyu commented Feb 26, 2020

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

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

dmnyu commented Mar 23, 2020

tbpg commented Mar 23, 2020

Choose a reason for hiding this comment

tbpg commented Mar 25, 2020

dmnyu commented Mar 26, 2020

tbpg commented Mar 26, 2020