You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "rliszli (via GitHub)" <gi...@apache.org> on 2023/11/13 14:23:23 UTC
[PR] NIFI-12351 - Enhance minifi java agent [nifi]
rliszli opened a new pull request, #8015:
URL: https://github.com/apache/nifi/pull/8015
<!-- Licensed to the Apache Software Foundation (ASF) under one or more -->
<!-- contributor license agreements. See the NOTICE file distributed with -->
<!-- this work for additional information regarding copyright ownership. -->
<!-- The ASF licenses this file to You under the Apache License, Version 2.0 -->
<!-- (the "License"); you may not use this file except in compliance with -->
<!-- the License. You may obtain a copy of the License at -->
<!-- http://www.apache.org/licenses/LICENSE-2.0 -->
<!-- Unless required by applicable law or agreed to in writing, software -->
<!-- distributed under the License is distributed on an "AS IS" BASIS, -->
<!-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -->
<!-- See the License for the specific language governing permissions and -->
<!-- limitations under the License. -->
# Summary
[NIFI-12351](https://issues.apache.org/jira/browse/NIFI-12351)
# Tracking
Please complete the following tracking steps prior to pull request creation.
### Issue Tracking
- [x] [Apache NiFi Jira](https://issues.apache.org/jira/browse/NIFI) issue created
### Pull Request Tracking
- [x] Pull Request title starts with Apache NiFi Jira issue number, such as `NIFI-00000`
- [x] Pull Request commit message starts with Apache NiFi Jira issue number, as such `NIFI-00000`
### Pull Request Formatting
- [x] Pull Request based on current revision of the `main` branch
- [ ] Pull Request refers to a feature branch with one commit containing changes
# Verification
Please indicate the verification steps performed prior to pull request creation.
### Build
- [x] Build completed using `mvn clean install -P contrib-check`
- [x] JDK 21
### Licensing
- [ ] New dependencies are compatible with the [Apache License 2.0](https://apache.org/licenses/LICENSE-2.0) according to the [License Policy](https://www.apache.org/legal/resolved.html)
- [ ] New dependencies are documented in applicable `LICENSE` and `NOTICE` files
### Documentation
- [ ] Documentation formatting appears as expected in rendered files
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12351 Improve MiNiFi Service Scripts for Windows [nifi]
Posted by "rliszli (via GitHub)" <gi...@apache.org>.
rliszli commented on PR #8015:
URL: https://github.com/apache/nifi/pull/8015#issuecomment-1818427167
Thanks for the suggestions and for the better naming. I added a short list for this PR containing the main elements of the change. I'll also update the `minifi-java-agent-quick-start.md` with a more detailed instructions on how to install as a service with the new scripts.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12351 Improve MiNiFi Service Scripts for Windows [nifi]
Posted by "bejancsaba (via GitHub)" <gi...@apache.org>.
bejancsaba commented on code in PR #8015:
URL: https://github.com/apache/nifi/pull/8015#discussion_r1404636904
##########
minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/install-service.bat:
##########
@@ -65,12 +125,21 @@ set LOG_PREFIX=minifi.log
--StopMethod="%STOP_METHOD%" ^
--StopTimeout="%STOP_TIMEOUT%" ^
--Classpath="%CLASS_PATH%" ^
---JvmOptions="%JAVA_ARGS%" ^
+--JvmOptions9="%JAVA_ARGS%" ^
Review Comment:
That was unexpected :) I read up on it so at least now I know what this is
##########
minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/delete-service.bat:
##########
@@ -16,8 +16,16 @@ rem See the License for the specific language governing permissions and
rem limitations under the License.
rem
-set SERVICE_NAME=minifi
-set SRV_BIN=%SERVICE_NAME%.exe
-
-REM Remove service
-%SRV_BIN% //DS//%SERVICE_NAME%
+setlocal enabledelayedexpansion
+
+set arg1=%1
+if "!arg1:~1,11!" equ "serviceName" (
Review Comment:
This pattern is repeated in this PR multiple times so I will just comment here.
Isn't there a cleaner way to do this on windows? I mean it clearly gets the job done but it is quite hard to read. While "findstr" has very limited pattern matching capabilities I think it should be able to handle this basic case or was there a reason for not using it? Also cutting the string along some delimiter (in this case the equal sign) would make it more robust as hardcoding positions can be super fragile. What do you think or you already tried these just was not working?
##########
minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/install-service.bat:
##########
@@ -16,13 +16,77 @@ rem See the License for the specific language governing permissions and
rem limitations under the License.
rem
-call minifi-env.bat
+call %~sdp0\minifi-env.bat
-set CONF_DIR=%MINIFI_ROOT%conf
+setlocal enabledelayedexpansion
+set "arg1="
+set "arg2="
+set "arg3="
+set "errorFlag=0"
+
+set "argCount=0"
+for %%A in (%*) do (
+ set /a "argCount+=1"
+
+ if !argCount! equ 1 set "arg1=%%~A"
+ if !argCount! equ 2 set "arg2=%%~A"
+ if !argCount! equ 3 set "arg3=%%~A"
+
+ if !argCount! geq 4 (
+ set "errorFlag=tooManyArguments"
+ goto :error
+ )
+
+ if "!arg1:~0,11!" equ "serviceName" (
Review Comment:
I think the indentation is not right here.
##########
minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/install-service.bat:
##########
@@ -16,13 +16,77 @@ rem See the License for the specific language governing permissions and
rem limitations under the License.
rem
-call minifi-env.bat
+call %~sdp0\minifi-env.bat
-set CONF_DIR=%MINIFI_ROOT%conf
+setlocal enabledelayedexpansion
+set "arg1="
+set "arg2="
+set "arg3="
+set "errorFlag=0"
+
+set "argCount=0"
+for %%A in (%*) do (
Review Comment:
Can we make this somewhat more intuitive, it is not straightforward what is happening here. There are at most 3 arguments here we could use the "%1", "%2" and "%3" notation match the argument to a variable and after this simple preprocessing I suppose the depth of the if / else below would be much more manageable. What do you think?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12351 - Enhance minifi java agent [nifi]
Posted by "bejancsaba (via GitHub)" <gi...@apache.org>.
bejancsaba commented on PR #8015:
URL: https://github.com/apache/nifi/pull/8015#issuecomment-1809680486
@rliszli Thanks for the change, without going deeper into the code aspect could you please add some details either to the ticket or the PR or even better to the README to explain what the change addresses, what is the intended behaviour change and how it can be validated?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12351 Improve MiNiFi Service Scripts for Windows [nifi]
Posted by "rliszli (via GitHub)" <gi...@apache.org>.
rliszli commented on PR #8015:
URL: https://github.com/apache/nifi/pull/8015#issuecomment-2031746756
@exceptionfactory: We still have one issue that needs to be addressed. I'll close the PR until it gets resolved.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12351 Improve MiNiFi Service Scripts for Windows [nifi]
Posted by "rliszli (via GitHub)" <gi...@apache.org>.
rliszli commented on PR #8015:
URL: https://github.com/apache/nifi/pull/8015#issuecomment-1845418710
`Thanks for the changes it is an area which clearly can benefit from some improvement. Maybe something is not right on my side but I'm getting the following`
Previously I only tested it on Windows 10. After this included Windows Server 2019 as well. Now should work on both.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12351 Improve MiNiFi Service Scripts for Windows [nifi]
Posted by "rliszli (via GitHub)" <gi...@apache.org>.
rliszli closed pull request #8015: NIFI-12351 Improve MiNiFi Service Scripts for Windows
URL: https://github.com/apache/nifi/pull/8015
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12351 Improve MiNiFi Service Scripts for Windows [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #8015:
URL: https://github.com/apache/nifi/pull/8015#issuecomment-1813705379
Concurring with @bejancsaba, it would be helpful to elaborate on the goal of these changes. I updated the pull request title to reflect that fact that the changes related to service scripts for MiNiFi on Windows, but it would be helpful to summarize the intended improvements.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12351 Improve MiNiFi Service Scripts for Windows [nifi]
Posted by "rliszli (via GitHub)" <gi...@apache.org>.
rliszli commented on code in PR #8015:
URL: https://github.com/apache/nifi/pull/8015#discussion_r1419023938
##########
minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/delete-service.bat:
##########
@@ -16,8 +16,16 @@ rem See the License for the specific language governing permissions and
rem limitations under the License.
rem
-set SERVICE_NAME=minifi
-set SRV_BIN=%SERVICE_NAME%.exe
-
-REM Remove service
-%SRV_BIN% //DS//%SERVICE_NAME%
+setlocal enabledelayedexpansion
+
+set arg1=%1
+if "!arg1:~1,11!" equ "serviceName" (
Review Comment:
Unfortunatelly I tried a lot of ways to do this. For example, the suggested _findstr_ cannot be applied inside an if condition. Honestly I don't like this either, as most part of how bat script working, but this was the "cleanest" or "simpliest" as I could find.
##########
minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/install-service.bat:
##########
@@ -16,13 +16,77 @@ rem See the License for the specific language governing permissions and
rem limitations under the License.
rem
-call minifi-env.bat
+call %~sdp0\minifi-env.bat
-set CONF_DIR=%MINIFI_ROOT%conf
+setlocal enabledelayedexpansion
+set "arg1="
+set "arg2="
+set "arg3="
+set "errorFlag=0"
+
+set "argCount=0"
+for %%A in (%*) do (
Review Comment:
Thanks, good idea, applied. Made the "code" much cleaner.
##########
minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/install-service.bat:
##########
@@ -16,13 +16,77 @@ rem See the License for the specific language governing permissions and
rem limitations under the License.
rem
-call minifi-env.bat
+call %~sdp0\minifi-env.bat
-set CONF_DIR=%MINIFI_ROOT%conf
+setlocal enabledelayedexpansion
+set "arg1="
+set "arg2="
+set "arg3="
+set "errorFlag=0"
+
+set "argCount=0"
+for %%A in (%*) do (
+ set /a "argCount+=1"
+
+ if !argCount! equ 1 set "arg1=%%~A"
+ if !argCount! equ 2 set "arg2=%%~A"
+ if !argCount! equ 3 set "arg3=%%~A"
+
+ if !argCount! geq 4 (
+ set "errorFlag=tooManyArguments"
+ goto :error
+ )
+
+ if "!arg1:~0,11!" equ "serviceName" (
Review Comment:
Thanks, should be fine now.
##########
minifi/minifi-nar-bundles/minifi-framework-bundle/minifi-framework/minifi-resources/src/main/resources/bin/install-service.bat:
##########
@@ -65,12 +125,21 @@ set LOG_PREFIX=minifi.log
--StopMethod="%STOP_METHOD%" ^
--StopTimeout="%STOP_TIMEOUT%" ^
--Classpath="%CLASS_PATH%" ^
---JvmOptions="%JAVA_ARGS%" ^
+--JvmOptions9="%JAVA_ARGS%" ^
Review Comment:
:)
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12351 Improve MiNiFi Service Scripts for Windows [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #8015:
URL: https://github.com/apache/nifi/pull/8015#issuecomment-1894866499
@rliszli This seems close to completion, aside from the one merge conflict. @bejancsaba I will defer to you for further review.
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12351 Improve MiNiFi Service Scripts for Windows [nifi]
Posted by "exceptionfactory (via GitHub)" <gi...@apache.org>.
exceptionfactory commented on PR #8015:
URL: https://github.com/apache/nifi/pull/8015#issuecomment-2019318631
@rliszli and @bejancsaba any updates on bringing this pull request to completion?
--
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: issues-unsubscribe@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org