Re: Review Request 52921: In immutable Thrift structs, check identity before comparing fields.

2016-10-17 Thread John Sirois 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52921/#review152893
---


Ship it!




Ship It!

- John Sirois


On Oct. 16, 2016, 5:04 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52921/
> ---
> 
> (Updated Oct. 16, 2016, 5:04 p.m.)
> 
> 
> Review request for Aurora and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> I saw THRIFT-3868 and thought we could apply the same micro-optimization
> as well. Details: https://github.com/apache/thrift/pull/1106
> 
> Example of a generated equals method (in ITaskConfig):
> 
> @Override
> public boolean equals(Object o) {
>   if (this == o) {
> return true;
>   }
>   if (!(o instanceof ITaskConfig)) {
> return false;
>   }
>   ITaskConfig other = (ITaskConfig) o;
>   return Objects.equals(job, other.job)
>   && Objects.equals(owner, other.owner)
>   && Objects.equals(isService, other.isService)
>   && Objects.equals(numCpus, other.numCpus)
>   && Objects.equals(ramMb, other.ramMb)
>   && Objects.equals(diskMb, other.diskMb)
>   && Objects.equals(priority, other.priority)
>   && Objects.equals(maxTaskFailures, other.maxTaskFailures)
>   && Objects.equals(production, other.production)
>   && Objects.equals(tier, other.tier)
>   && Objects.equals(resources, other.resources)
>   && Objects.equals(constraints, other.constraints)
>   && Objects.equals(requestedPorts, other.requestedPorts)
>   && Objects.equals(mesosFetcherUris, other.mesosFetcherUris)
>   && Objects.equals(taskLinks, other.taskLinks)
>   && Objects.equals(contactEmail, other.contactEmail)
>   && Objects.equals(executorConfig, other.executorConfig)
>   && Objects.equals(metadata, other.metadata)
>   && Objects.equals(container, other.container);
> }
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py 
> 7c281800b00b973351659ecacd5b7f6e55e46bba 
> 
> Diff: https://reviews.apache.org/r/52921/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 52921: In immutable Thrift structs, check identity before comparing fields.

2016-10-17 Thread John Sirois 


> On Oct. 17, 2016, 9:41 a.m., Joshua Cohen wrote:
> > Should we also add:
> > 
> > if (o == null) {
> >   return false;
> > }
> > 
> > ?
> > 
> > Curious if we're able to see a difference in benchmarks from this change?

It would be a bit of black magic if that were significantly faster than the 
!instanceof check.


- John


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52921/#review152889
---


On Oct. 16, 2016, 5:04 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52921/
> ---
> 
> (Updated Oct. 16, 2016, 5:04 p.m.)
> 
> 
> Review request for Aurora and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> I saw THRIFT-3868 and thought we could apply the same micro-optimization
> as well. Details: https://github.com/apache/thrift/pull/1106
> 
> Example of a generated equals method (in ITaskConfig):
> 
> @Override
> public boolean equals(Object o) {
>   if (this == o) {
> return true;
>   }
>   if (!(o instanceof ITaskConfig)) {
> return false;
>   }
>   ITaskConfig other = (ITaskConfig) o;
>   return Objects.equals(job, other.job)
>   && Objects.equals(owner, other.owner)
>   && Objects.equals(isService, other.isService)
>   && Objects.equals(numCpus, other.numCpus)
>   && Objects.equals(ramMb, other.ramMb)
>   && Objects.equals(diskMb, other.diskMb)
>   && Objects.equals(priority, other.priority)
>   && Objects.equals(maxTaskFailures, other.maxTaskFailures)
>   && Objects.equals(production, other.production)
>   && Objects.equals(tier, other.tier)
>   && Objects.equals(resources, other.resources)
>   && Objects.equals(constraints, other.constraints)
>   && Objects.equals(requestedPorts, other.requestedPorts)
>   && Objects.equals(mesosFetcherUris, other.mesosFetcherUris)
>   && Objects.equals(taskLinks, other.taskLinks)
>   && Objects.equals(contactEmail, other.contactEmail)
>   && Objects.equals(executorConfig, other.executorConfig)
>   && Objects.equals(metadata, other.metadata)
>   && Objects.equals(container, other.container);
> }
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py 
> 7c281800b00b973351659ecacd5b7f6e55e46bba 
> 
> Diff: https://reviews.apache.org/r/52921/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 52921: In immutable Thrift structs, check identity before comparing fields.

2016-10-17 Thread Joshua Cohen 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52921/#review152889
---



Should we also add:

if (o == null) {
  return false;
}

?

Curious if we're able to see a difference in benchmarks from this change?

- Joshua Cohen


On Oct. 16, 2016, 11:04 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52921/
> ---
> 
> (Updated Oct. 16, 2016, 11:04 p.m.)
> 
> 
> Review request for Aurora and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> I saw THRIFT-3868 and thought we could apply the same micro-optimization
> as well. Details: https://github.com/apache/thrift/pull/1106
> 
> Example of a generated equals method (in ITaskConfig):
> 
> @Override
> public boolean equals(Object o) {
>   if (this == o) {
> return true;
>   }
>   if (!(o instanceof ITaskConfig)) {
> return false;
>   }
>   ITaskConfig other = (ITaskConfig) o;
>   return Objects.equals(job, other.job)
>   && Objects.equals(owner, other.owner)
>   && Objects.equals(isService, other.isService)
>   && Objects.equals(numCpus, other.numCpus)
>   && Objects.equals(ramMb, other.ramMb)
>   && Objects.equals(diskMb, other.diskMb)
>   && Objects.equals(priority, other.priority)
>   && Objects.equals(maxTaskFailures, other.maxTaskFailures)
>   && Objects.equals(production, other.production)
>   && Objects.equals(tier, other.tier)
>   && Objects.equals(resources, other.resources)
>   && Objects.equals(constraints, other.constraints)
>   && Objects.equals(requestedPorts, other.requestedPorts)
>   && Objects.equals(mesosFetcherUris, other.mesosFetcherUris)
>   && Objects.equals(taskLinks, other.taskLinks)
>   && Objects.equals(contactEmail, other.contactEmail)
>   && Objects.equals(executorConfig, other.executorConfig)
>   && Objects.equals(metadata, other.metadata)
>   && Objects.equals(container, other.container);
> }
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py 
> 7c281800b00b973351659ecacd5b7f6e55e46bba 
> 
> Diff: https://reviews.apache.org/r/52921/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 52921: In immutable Thrift structs, check identity before comparing fields.

2016-10-16 Thread Aurora ReviewBot 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52921/#review152814
---



Master (ac8b802) is green with this patch.
  ./build-support/jenkins/build.sh

However, it appears that it might lack test coverage.

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Oct. 16, 2016, 11:04 p.m., Stephan Erb wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52921/
> ---
> 
> (Updated Oct. 16, 2016, 11:04 p.m.)
> 
> 
> Review request for Aurora and John Sirois.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> I saw THRIFT-3868 and thought we could apply the same micro-optimization
> as well. Details: https://github.com/apache/thrift/pull/1106
> 
> Example of a generated equals method (in ITaskConfig):
> 
> @Override
> public boolean equals(Object o) {
>   if (this == o) {
> return true;
>   }
>   if (!(o instanceof ITaskConfig)) {
> return false;
>   }
>   ITaskConfig other = (ITaskConfig) o;
>   return Objects.equals(job, other.job)
>   && Objects.equals(owner, other.owner)
>   && Objects.equals(isService, other.isService)
>   && Objects.equals(numCpus, other.numCpus)
>   && Objects.equals(ramMb, other.ramMb)
>   && Objects.equals(diskMb, other.diskMb)
>   && Objects.equals(priority, other.priority)
>   && Objects.equals(maxTaskFailures, other.maxTaskFailures)
>   && Objects.equals(production, other.production)
>   && Objects.equals(tier, other.tier)
>   && Objects.equals(resources, other.resources)
>   && Objects.equals(constraints, other.constraints)
>   && Objects.equals(requestedPorts, other.requestedPorts)
>   && Objects.equals(mesosFetcherUris, other.mesosFetcherUris)
>   && Objects.equals(taskLinks, other.taskLinks)
>   && Objects.equals(contactEmail, other.contactEmail)
>   && Objects.equals(executorConfig, other.executorConfig)
>   && Objects.equals(metadata, other.metadata)
>   && Objects.equals(container, other.container);
> }
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py 
> 7c281800b00b973351659ecacd5b7f6e55e46bba 
> 
> Diff: https://reviews.apache.org/r/52921/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Stephan Erb
> 
>

Re: Review Request 52921: In immutable Thrift structs, check identity before comparing fields.

2016-10-16 Thread Stephan Erb 

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52921/
---

(Updated Oct. 17, 2016, 1:04 a.m.)


Review request for Aurora.


Repository: aurora


Description (updated)
---

I saw THRIFT-3868 and thought we could apply the same micro-optimization
as well. Details: https://github.com/apache/thrift/pull/1106

Example of a generated equals method (in ITaskConfig):

@Override
public boolean equals(Object o) {
  if (this == o) {
return true;
  }
  if (!(o instanceof ITaskConfig)) {
return false;
  }
  ITaskConfig other = (ITaskConfig) o;
  return Objects.equals(job, other.job)
  && Objects.equals(owner, other.owner)
  && Objects.equals(isService, other.isService)
  && Objects.equals(numCpus, other.numCpus)
  && Objects.equals(ramMb, other.ramMb)
  && Objects.equals(diskMb, other.diskMb)
  && Objects.equals(priority, other.priority)
  && Objects.equals(maxTaskFailures, other.maxTaskFailures)
  && Objects.equals(production, other.production)
  && Objects.equals(tier, other.tier)
  && Objects.equals(resources, other.resources)
  && Objects.equals(constraints, other.constraints)
  && Objects.equals(requestedPorts, other.requestedPorts)
  && Objects.equals(mesosFetcherUris, other.mesosFetcherUris)
  && Objects.equals(taskLinks, other.taskLinks)
  && Objects.equals(contactEmail, other.contactEmail)
  && Objects.equals(executorConfig, other.executorConfig)
  && Objects.equals(metadata, other.metadata)
  && Objects.equals(container, other.container);
}


Diffs
-

  src/main/python/apache/aurora/tools/java/thrift_wrapper_codegen.py 
7c281800b00b973351659ecacd5b7f6e55e46bba 

Diff: https://reviews.apache.org/r/52921/diff/


Testing
---

./gradlew -Pq build


Thanks,

Stephan Erb