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

check cache before referring to peer #20

Merged
merged 9 commits into from
Sep 1, 2020
Merged

Conversation

golanha
Copy link
Member

@golanha golanha commented Aug 25, 2020

This change is Reviewable

@golanha golanha requested review from nassiharel and yehiyam and removed request for nassiharel August 25, 2020 13:17
Copy link
Contributor

@nassiharel nassiharel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @golanha and @yehiyam)


hkube_python_wrapper/wrapper/data_adapter.py, line 25 at r2 (raw file):

 print('using {workers} for DataAdapter'.format(
            workers=self._maxWorkers))

remove this new line format from the IDE....
The code looks very strange...


hkube_python_wrapper/wrapper/data_adapter.py, line 177 at r2 (raw file):

if (self._dataServer and self._dataServer.isLocal(host, port)):

What about this use-case?

Copy link
Contributor

@yehiyam yehiyam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @golanha)


setup.py, line 25 at r2 (raw file):

    "pyzmq",
    "jaeger-client==4",
    "pympler"

didn't we remove it? maybe you need to update from master

@golanha
Copy link
Member Author

golanha commented Aug 26, 2020


setup.py, line 25 at r2 (raw file):

Previously, yehiyam wrote…

didn't we remove it? maybe you need to update from master

I put it back.
because now I put data that I got from peer to cache.
Generally I can check the size of what I get from peer before I decode it.
But in one case, where I ask peer for data of tasks (many), I get results of many tasks together. I decode it and add each separately to cache. I need to use the asizeof() to determine the size of each result.

Copy link
Member Author

@golanha golanha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @nassiharel and @yehiyam)


setup.py, line 25 at r2 (raw file):

Previously, golanha (Golan Hallel) wrote…

I put it back.
because now I put data that I got from peer to cache.
Generally I can check the size of what I get from peer before I decode it.
But in one case, where I ask peer for data of tasks (many), I get results of many tasks together. I decode it and add each separately to cache. I need to use the asizeof() to determine the size of each result.

Done.


hkube_python_wrapper/wrapper/data_adapter.py, line 25 at r2 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…
 print('using {workers} for DataAdapter'.format(
            workers=self._maxWorkers))

remove this new line format from the IDE....
The code looks very strange...

Done.


hkube_python_wrapper/wrapper/data_adapter.py, line 177 at r2 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…
if (self._dataServer and self._dataServer.isLocal(host, port)):

What about this use-case?

You are right.
In this case, I will set the size to 0, since its already taken in to consideration as part of the DataServer cache.

Copy link
Member Author

@golanha golanha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 5 files reviewed, 3 unresolved discussions (waiting on @nassiharel and @yehiyam)


hkube_python_wrapper/wrapper/data_adapter.py, line 177 at r2 (raw file):

Previously, golanha (Golan Hallel) wrote…

You are right.
In this case, I will set the size to 0, since its already taken in to consideration as part of the DataServer cache.

On second thought, I will not add it to cache at all since later on , it may be removed from the DataServer cache, and not calculated as part of the in storagecache.

Copy link
Contributor

@nassiharel nassiharel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3.
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @golanha, @nassiharel, and @yehiyam)


hkube_python_wrapper/wrapper/data_adapter.py, line 113 at r4 (raw file):

for t in tasks:

is it iterates all tasks or tasksNotInCache?

Copy link
Member Author

@golanha golanha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @nassiharel and @yehiyam)


hkube_python_wrapper/wrapper/data_adapter.py, line 113 at r4 (raw file):

for t in tasks

Should be tasksNotInCache, I'll correct it.

Copy link
Contributor

@nassiharel nassiharel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @golanha and @yehiyam)


hkube_python_wrapper/wrapper/data_adapter.py, line 86 at r5 (raw file):

pylint: disable=too-many-branches

Pylint is right, it has become too complicated and it will be very hard to do unit-tests with full coverage.

Copy link
Member Author

@golanha golanha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 3 of 6 files reviewed, 2 unresolved discussions (waiting on @nassiharel and @yehiyam)


hkube_python_wrapper/wrapper/data_adapter.py, line 86 at r5 (raw file):

Previously, NassiHarel (Nassi Harel) wrote…
pylint: disable=too-many-branches

Pylint is right, it has become too complicated and it will be very hard to do unit-tests with full coverage.

Done.

Copy link
Contributor

@nassiharel nassiharel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 3 of 3 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yehiyam)

@golanha golanha merged commit 7997a12 into master Sep 1, 2020
hkube-ci pushed a commit that referenced this pull request Sep 1, 2020
check cache before referring to peer .... bump version [skip ci]
@nassiharel nassiharel deleted the checkCacheBeforePeer branch September 8, 2020 10:50
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.

None yet

3 participants