[GitHub] [storm] govind-menon commented on pull request #3281: STORM-3647: Adds OFF HEAP to worker child opts

[GitBox]

govind-menon commented on pull request #3281:
URL: https://github.com/apache/storm/pull/3281#issuecomment-639120389


   Yes that is correct



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [storm] Ethanlm commented on pull request #3281: STORM-3647: Adds OFF HEAP to worker child opts

[GitBox]

Ethanlm commented on pull request #3281:
URL: https://github.com/apache/storm/pull/3281#issuecomment-639118251


   Yes but if worker.childopts doesn't have `%OFF-HEAP-MEM%`, this setting is 
not really enforced, right? 



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [storm] govind-menon commented on pull request #3281: STORM-3647: Adds OFF HEAP to worker child opts

[GitBox]

govind-menon commented on pull request #3281:
URL: https://github.com/apache/storm/pull/3281#issuecomment-639112741


   The off heap memory requested for the executor is what is put in the flag



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [storm] Ethanlm edited a comment on pull request #3281: STORM-3647: Adds OFF HEAP to worker child opts

[GitBox]

Ethanlm edited a comment on pull request #3281:
URL: https://github.com/apache/storm/pull/3281#issuecomment-639111921


   How can a topology use off heap memory?  %OFF-HEAP-MEM% is not really used 
in the template `worker.childopts`



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [storm] Ethanlm commented on pull request #3281: STORM-3647: Adds OFF HEAP to worker child opts

[GitBox]

Ethanlm commented on pull request #3281:
URL: https://github.com/apache/storm/pull/3281#issuecomment-639111921


   How can a topology use off heap memory? Setting %OFF-HEAP-MEM% doesn't seem 
enough?



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [storm] govind-menon commented on pull request #3281: STORM-3647: Adds OFF HEAP to worker child opts

[GitBox]

govind-menon commented on pull request #3281:
URL: https://github.com/apache/storm/pull/3281#issuecomment-639103137


   @Ethanlm Done.



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [storm] Ethanlm commented on pull request #3281: STORM-3647: Adds OFF HEAP to worker child opts

[GitBox]

Ethanlm commented on pull request #3281:
URL: https://github.com/apache/storm/pull/3281#issuecomment-639090241


   please update commit message to also include the jira id



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [storm] govind-menon opened a new pull request #3281: Adds OFF HEAP to worker child opts

[GitBox]

govind-menon opened a new pull request #3281:
URL: https://github.com/apache/storm/pull/3281


   ## What is the purpose of the change
   
   *(Explain why we should have this change)*
   
   ## How was the change tested
   
   *(Explain what tests did you do to verify the code change)*



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [storm] Ethanlm commented on a change in pull request #3274: [STORM-3639] Replace asserts in daemon code.

[GitBox]

Ethanlm commented on a change in pull request #3274:
URL: https://github.com/apache/storm/pull/3274#discussion_r435449649



##
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##
@@ -814,7 +814,12 @@ private static int numUsedWorkers(SchedulerAssignment 
assignment) {
 
 private boolean auditAssignmentChanges(Map 
existingAssignments,
Map 
newAssignments) {
-assert existingAssignments != null && newAssignments != null;
+if (existingAssignments == null) {

Review comment:
   Sorry I am a little confused. So 
   
   What's the difference between 
   `assert existingAssignments != null && newAssignments != null;`
   and 
   `assert (TopologyStatus.ACTIVE == initStatus || TopologyStatus.INACTIVE == 
initStatus);`
   
   Why the first one should be deleted and the second should be changed?





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [VOTE] Release Apache Storm 2.2.0

[Ethan Li]

The fix is ready. Please review https://github.com/apache/storm/pull/3280 


> On Jun 4, 2020, at 11:34 AM, Kishorkumar Patil  
> wrote:
> 
> Hi Julian,
> 
> I agree with you on this. Ethan has a fix that will resolve both these
> issues. We should get the change in and cut another RC.
> 
> -Kishor
> 
> 
> On Thu, Jun 4, 2020 at 12:23 PM Julien Nioche 
> wrote:
> 
>> Hi,
>> 
>> Great to see that there has been some work on Storm 2 recently but I am
>> afraid I'll have to cast a (non-binding) -1 because of STORM-3620
>>  and STORM-3582
>> .
>> 
>> I have tried to get StormCrawler to work with Storm 2.x but the issues
>> above have prevented me from doing so. At least one other person has had
>> the same problems as you can see in the JIRAs above.
>> 
>> I think these are serious issues and they should be addressed in the next
>> release. I would be very happy to help if I can.
>> 
>> Julien
>> 
>> On Thu, 4 Jun 2020 at 00:00, Govind Menon  wrote:
>> 
>>> This is a call to vote on releasing Apache Storm 2.2.0 (As a personal
>> note
>>> I would like to thank P. Taylor Goetz and Ethan Li for all the
>>> documentation they have set up to make releasing easier)
>>> 
>>> Full list of changes in this release:
>>> 
>>> 
>>> 
>> https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.2.0/RELEASE_NOTES.html
>>> 
>>> The tag/commit to be voted upon is v2.2.0:
>>> 
>>> 
>>> 
>> https://gitbox.apache.org/repos/asf?p=storm.git;a=commit;h=6326b0c783c0025440c629e17653c0c7577f27e6
>>> 
>>> The source archive being voted upon can be found here:
>>> 
>>> 
>>> 
>> https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.2.0/apache-storm-2.2.0-src.tar.gz
>>> 
>>> Other release files, signatures and digests can be found here:
>>> 
>>> https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.2.0/
>>> 
>>> The release artifacts are signed with the following key:
>>> 
>>> https://www.apache.org/dist/storm/KEYS
>>> 
>>> The Nexus staging repository for this release is:
>>> 
>>> https://repository.apache.org/content/repositories/orgapachestorm-1091
>>> 
>>> Please vote on releasing this package as Apache Storm 2.2.0.
>>> 
>>> When voting, please list the actions taken to verify the release.
>>> 
>>> This vote will be open for at least 72 hours.
>>> 
>>> [ ] +1 Release this package as Apache Storm 2.2.0
>>> [ ]  0 No opinion
>>> [ ] -1 Do not release this package because...
>>> 
>>> Thanks to everyone who contributed to this release.
>>> 
>> 
>> 
>> --
>> 
>> *Open Source Solutions for Text Engineering*
>> 
>> http://www.digitalpebble.com
>> http://digitalpebble.blogspot.com/
>> #digitalpebble 
>> 

[GitHub] [storm] Ethanlm opened a new pull request #3280: [STORM-3620] fix data corruption in the case of multi-threaded bolts …

[GitBox]

Ethanlm opened a new pull request #3280:
URL: https://github.com/apache/storm/pull/3280


   ## What is the purpose of the change
   
   Users might want to launch extra threads within bolts ; because the 
serializer is not thread safe, there will be race conditions causing data 
corruption. The change here is to make the serializer thread-local, add unit 
test, and add a MultiThreadWordCount example. 
   
   ## How was the change tested
   
   1. Added unit test
   2. submit the MultiThreadWordCount example and validated that the 
threadlocal serializer fixed the data corruption issue.
   
   
   There is another known issue 
https://issues.apache.org/jira/browse/STORM-3646 when 
   ```
   topology.producer.batch.size
   topology.transfer.batch.size
   ```
   are not 1 (by default is 1). Make sure don't change the values to something 
else for now. 



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [storm] bipinprasad commented on a change in pull request #3274: [STORM-3639] Replace asserts in daemon code.

[GitBox]

bipinprasad commented on a change in pull request #3274:
URL: https://github.com/apache/storm/pull/3274#discussion_r435431019



##
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##
@@ -814,7 +814,12 @@ private static int numUsedWorkers(SchedulerAssignment 
assignment) {
 
 private boolean auditAssignmentChanges(Map 
existingAssignments,
Map 
newAssignments) {
-assert existingAssignments != null && newAssignments != null;
+if (existingAssignments == null) {

Review comment:
   Definitely need critical review of code that is throwing unchecked 
exception (including AssertionError()) and look for options  - or complete 
removal - even in the changed code. 
   
   At best, "assert" usage should be viewed as lazy coding in private methods. 
And should not be used in protected or public methods.
   https://docs.oracle.com/javase/7/docs/technotes/guides/language/assert.html 
(note the language that it should not be used in public methods).
   
   This does not mean that private methods cannot throw Exceptions. They can 
and very often do. But those are checked exceptions (i.e. present in method 
signature).
   
   The goal of this change is to remove asserts. Replace with proper checking 
where possible, throw checked Exception where appropriate. Reason is quite 
straightforward - assert statements may not be executed and gives a false 
impression that something is being done.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [VOTE] Release Apache Storm 2.2.0

[Kishorkumar Patil]

Hi Julian,

I agree with you on this. Ethan has a fix that will resolve both these
issues. We should get the change in and cut another RC.

-Kishor


On Thu, Jun 4, 2020 at 12:23 PM Julien Nioche 
wrote:

> Hi,
>
> Great to see that there has been some work on Storm 2 recently but I am
> afraid I'll have to cast a (non-binding) -1 because of STORM-3620
>  and STORM-3582
> .
>
> I have tried to get StormCrawler to work with Storm 2.x but the issues
> above have prevented me from doing so. At least one other person has had
> the same problems as you can see in the JIRAs above.
>
> I think these are serious issues and they should be addressed in the next
> release. I would be very happy to help if I can.
>
> Julien
>
> On Thu, 4 Jun 2020 at 00:00, Govind Menon  wrote:
>
> > This is a call to vote on releasing Apache Storm 2.2.0 (As a personal
> note
> > I would like to thank P. Taylor Goetz and Ethan Li for all the
> > documentation they have set up to make releasing easier)
> >
> > Full list of changes in this release:
> >
> >
> >
> https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.2.0/RELEASE_NOTES.html
> >
> > The tag/commit to be voted upon is v2.2.0:
> >
> >
> >
> https://gitbox.apache.org/repos/asf?p=storm.git;a=commit;h=6326b0c783c0025440c629e17653c0c7577f27e6
> >
> > The source archive being voted upon can be found here:
> >
> >
> >
> https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.2.0/apache-storm-2.2.0-src.tar.gz
> >
> > Other release files, signatures and digests can be found here:
> >
> > https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.2.0/
> >
> > The release artifacts are signed with the following key:
> >
> > https://www.apache.org/dist/storm/KEYS
> >
> > The Nexus staging repository for this release is:
> >
> > https://repository.apache.org/content/repositories/orgapachestorm-1091
> >
> > Please vote on releasing this package as Apache Storm 2.2.0.
> >
> > When voting, please list the actions taken to verify the release.
> >
> > This vote will be open for at least 72 hours.
> >
> > [ ] +1 Release this package as Apache Storm 2.2.0
> > [ ]  0 No opinion
> > [ ] -1 Do not release this package because...
> >
> > Thanks to everyone who contributed to this release.
> >
>
>
> --
>
> *Open Source Solutions for Text Engineering*
>
> http://www.digitalpebble.com
> http://digitalpebble.blogspot.com/
> #digitalpebble 
>

Re: [VOTE] Release Apache Storm 2.2.0

[Julien Nioche]

Hi,

Great to see that there has been some work on Storm 2 recently but I am
afraid I'll have to cast a (non-binding) -1 because of STORM-3620
 and STORM-3582
.

I have tried to get StormCrawler to work with Storm 2.x but the issues
above have prevented me from doing so. At least one other person has had
the same problems as you can see in the JIRAs above.

I think these are serious issues and they should be addressed in the next
release. I would be very happy to help if I can.

Julien

On Thu, 4 Jun 2020 at 00:00, Govind Menon  wrote:

> This is a call to vote on releasing Apache Storm 2.2.0 (As a personal note
> I would like to thank P. Taylor Goetz and Ethan Li for all the
> documentation they have set up to make releasing easier)
>
> Full list of changes in this release:
>
>
> https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.2.0/RELEASE_NOTES.html
>
> The tag/commit to be voted upon is v2.2.0:
>
>
> https://gitbox.apache.org/repos/asf?p=storm.git;a=commit;h=6326b0c783c0025440c629e17653c0c7577f27e6
>
> The source archive being voted upon can be found here:
>
>
> https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.2.0/apache-storm-2.2.0-src.tar.gz
>
> Other release files, signatures and digests can be found here:
>
> https://dist.apache.org/repos/dist/dev/storm/apache-storm-2.2.0/
>
> The release artifacts are signed with the following key:
>
> https://www.apache.org/dist/storm/KEYS
>
> The Nexus staging repository for this release is:
>
> https://repository.apache.org/content/repositories/orgapachestorm-1091
>
> Please vote on releasing this package as Apache Storm 2.2.0.
>
> When voting, please list the actions taken to verify the release.
>
> This vote will be open for at least 72 hours.
>
> [ ] +1 Release this package as Apache Storm 2.2.0
> [ ]  0 No opinion
> [ ] -1 Do not release this package because...
>
> Thanks to everyone who contributed to this release.
>


-- 

*Open Source Solutions for Text Engineering*

http://www.digitalpebble.com
http://digitalpebble.blogspot.com/
#digitalpebble 

[GitHub] [storm] Ethanlm commented on a change in pull request #3274: [STORM-3639] Replace asserts in daemon code.

[GitBox]

Ethanlm commented on a change in pull request #3274:
URL: https://github.com/apache/storm/pull/3274#discussion_r435339674



##
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##
@@ -814,7 +814,12 @@ private static int numUsedWorkers(SchedulerAssignment 
assignment) {
 
 private boolean auditAssignmentChanges(Map 
existingAssignments,
Map 
newAssignments) {
-assert existingAssignments != null && newAssignments != null;
+if (existingAssignments == null) {

Review comment:
   Your second point about defensive programming kinda makes sense to me.
   
   For the first point, "no reason to throw unchecked exceptions in a private 
method because it is totally in control of the class scope", I don't disagree 
with that. But with this assumption/standard, there are changes in this PR that 
can be removed too since some of them are also this case, for example, 
   `if (TopologyStatus.ACTIVE != initStatus && TopologyStatus.INACTIVE != 
initStatus) `. 
   
And I think those places are where we can use `assert`. My understanding of 
using `assert` is to detect mistakes developers make during development. Since 
we have total control of a private method, we don't expect those `null` or 
invalid case to really happen. So we detect those in development, but not in 
production.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [storm] Ethanlm merged pull request #3279: STORM-3644 update pacemaker connection error log

[GitBox]

Ethanlm merged pull request #3279:
URL: https://github.com/apache/storm/pull/3279


   



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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [storm] bipinprasad commented on a change in pull request #3274: [STORM-3639] Replace asserts in daemon code.

[GitBox]

bipinprasad commented on a change in pull request #3274:
URL: https://github.com/apache/storm/pull/3274#discussion_r435267750



##
File path: storm-server/src/main/java/org/apache/storm/daemon/nimbus/Nimbus.java
##
@@ -814,7 +814,12 @@ private static int numUsedWorkers(SchedulerAssignment 
assignment) {
 
 private boolean auditAssignmentChanges(Map 
existingAssignments,
Map 
newAssignments) {
-assert existingAssignments != null && newAssignments != null;
+if (existingAssignments == null) {

Review comment:
   Throwing unchecked exceptions is generally not considered good practice. 
So using Preconditions would be not be considered good practice. Unchecked 
Exceptions should be thrown only in rare cases. In this case, it is a private 
method, totally in control of the class scope, and therefore no reason to throw 
unchecked exceptions. 
   
   With regard to checking for null - that will not be considered defensive 
programming. Defense would be to check for null and then do something about it 
- i.e. set to to an empty list instead. The idea being that the method can 
handle parameters can can be "bad" in some sense. In this specific private 
method, that defense is not required.
   
   Throwing unchecked exception is not a defense. 
   last few lines here: 
https://docs.oracle.com/javase/tutorial/essential/exceptions/runtime.html





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org