Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jobspec: replace minus operator to multiplication #1293

Merged
merged 1 commit into from
Nov 28, 2017
Merged

jobspec: replace minus operator to multiplication #1293

merged 1 commit into from
Nov 28, 2017

Conversation

dongahn
Copy link
Member

@dongahn dongahn commented Nov 23, 2017

Make the implementation come to compliance with RFC14's JRC rule section.
Tested with resource-query and this one-liner seems enough.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 78.614% when pulling 036c631 on dongahn:rfc_compliance into a2b1063 on flux-framework:master.

@codecov-io
Copy link

codecov-io commented Nov 23, 2017

Codecov Report

Merging #1293 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1293      +/-   ##
==========================================
- Coverage   78.02%   78.01%   -0.02%     
==========================================
  Files         154      154              
  Lines       29104    29104              
==========================================
- Hits        22709    22706       -3     
- Misses       6395     6398       +3
Impacted Files Coverage Δ
src/common/libflux/keepalive.c 86.66% <0%> (-6.67%) ⬇️
src/broker/modservice.c 79.61% <0%> (-1.95%) ⬇️
src/modules/connector-local/local.c 76.11% <0%> (-1.31%) ⬇️
src/common/libflux/response.c 83.73% <0%> (-0.82%) ⬇️
src/common/libflux/handle.c 83.66% <0%> (-0.5%) ⬇️
src/common/libflux/future.c 88.31% <0%> (-0.47%) ⬇️
src/common/libflux/message.c 81.36% <0%> (+0.23%) ⬆️
src/common/libkvs/kvs.c 66.54% <0%> (+0.24%) ⬆️
src/modules/kvs/kvs.c 64.95% <0%> (+0.25%) ⬆️
src/common/libkvs/treeobj.c 85.54% <0%> (+0.4%) ⬆️
... and 3 more

@dongahn
Copy link
Member Author

dongahn commented Nov 24, 2017

Hmmm. I don't understand why this one line change causes a valgrind-related error....

https://travis-ci.org/flux-framework/flux-core/jobs/306163007#L9029

@garlick
Copy link
Member

garlick commented Nov 24, 2017

Looks like an unrelated transient error in czmq shutdown:

==10045== Memcheck, a memory error detector
==10045== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==10045== Using Valgrind-3.10.1 and LibVEX; rerun with -h for copyright info
==10045== Command: /home/travis/build/flux-framework/flux-core/flux-core-0.8.0-465-g95b20d2f/_build/sub/src/broker/.libs/lt-flux-broker --shutdown-grace=4 /home/travis/build/flux-framework/flux-core/flux-core-0.8.0-465-g95b20d2f/t/valgrind/valgrind-workload.sh 10
==10045== 
FLUX_URI=local:///tmp/flux-5SBsSk
==10045== 
==10045== HEAP SUMMARY:
==10045==     in use at exit: 111,117 bytes in 102 blocks
==10045==   total heap usage: 912,236 allocs, 912,134 frees, 78,583,150 bytes allocated
==10045== 
==10045== 288 bytes in 1 blocks are possibly lost in loss record 27 of 39
==10045==    at 0x4C2CC70: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==10045==    by 0x4012FE4: allocate_dtv (dl-tls.c:296)
==10045==    by 0x4012FE4: _dl_allocate_tls (dl-tls.c:460)
==10045==    by 0x5E3ED92: allocate_stack (allocatestack.c:589)
==10045==    by 0x5E3ED92: pthread_create@@GLIBC_2.2.5 (pthread_create.c:500)
==10045==    by 0x54CD8DA: zactor_new (zactor.c:122)
==10045==    by 0x4E4826A: flux_sec_comms_init (security.c:197)
==10045==    by 0x407361: main (broker.c:411)
==10045== 
{
   <insert_a_suppression_name_here>
   Memcheck:Leak
   match-leak-kinds: possible
   fun:calloc
   fun:allocate_dtv
   fun:_dl_allocate_tls
   fun:allocate_stack
   fun:pthread_create@@GLIBC_2.2.5
   fun:zactor_new
   fun:flux_sec_comms_init
   fun:main
}
==10045== LEAK SUMMARY:
==10045==    definitely lost: 0 bytes in 0 blocks
==10045==    indirectly lost: 0 bytes in 0 blocks
==10045==      possibly lost: 288 bytes in 1 blocks
==10045==    still reachable: 110,317 bytes in 100 blocks
==10045==         suppressed: 512 bytes in 1 blocks
==10045== Reachable blocks (those to which a pointer was found) are not shown.
==10045== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==10045== 
==10045== For counts of detected and suppressed errors, rerun with: -v
==10045== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 1 from 1)
not ok 1 - valgrind reports no new errors on single broker run

@garlick
Copy link
Member

garlick commented Nov 24, 2017

@dongahn: it would be nice to have a more descriptive commit message, for those of us who have not been following the jobspec design in detail.

jobspec: replace minus operator to multiplication

Did you mean "replace minus operator with multiplcation"?

Make the implementation come to compliance with RFC14's JRC rule section.
Tested with resource-query and this one-liner seems enough.

What specifically is out of compliance? Perhaps a quote from the spec would be useful here.

Make the implementation come to compliance with RFC14's Content
Rule section. It currently does not have the subtraction operator
listed, but multiplication:

===
Content Rules

$complex_range = {
    "min" : 1..,
    "max" : 1..,
    "operator" : ( :"+" | :"*" | : "^" ),
    "operand" : 1..,
}
===

The role of an operator is:
An operator applied between min and max which returns the next
acceptable value.
It seems using an operator that can return a decreased
or the same next value doesn't make whole lot of sense to me.

Tested with resource-query and this one-liner seems enough.
@dongahn
Copy link
Member Author

dongahn commented Nov 24, 2017

@garlick: OK, that makes sense. I forced a push. Hopefully this works.

@garlick
Copy link
Member

garlick commented Nov 24, 2017

Much better, thank you!

Looks like you may need to rebase on current master?

We probably need @morrone or @grondo to give an ACK before merging.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 78.597% when pulling ad747cf on dongahn:rfc_compliance into a2b1063 on flux-framework:master.

@garlick
Copy link
Member

garlick commented Nov 24, 2017

Looks like you may need to rebase on current master?

Strike that - github was saying it was out of date, but not now.

@grondo
Copy link
Contributor

grondo commented Nov 24, 2017

Nice explanation @dongahn, makes sense to me! (Was likely just a typo in the orignal)

My only suggestion is maybe we should add a test case to t/jobspec/invalid to ensure a jobspec input using the invalid operator - is rejected?

@dongahn
Copy link
Member Author

dongahn commented Nov 25, 2017

ok. should be easy enough.

@morrone morrone merged commit fab9d9e into flux-framework:master Nov 28, 2017
@grondo grondo mentioned this pull request May 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants