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

HPCC-11522 libmemcached plugin #6368

Closed

Conversation

jamienoss
Copy link
Collaborator

Signed-off-by: jamienoss james.noss@lexisnexis.com

@hpcc-jirabot
Copy link

Jira updated

@jamienoss
Copy link
Collaborator Author

@richardkchapman @ghalliday please review.
Wasn't sure who else to add.

FIND_PATH(LIBMEMCACHED_INCLUDE_DIR memcached.hpp PATHS /usr/include /usr/share/include /usr/local/include PATH_SUFFIXES libmemcached-1.0)

FIND_LIBRARY (LIBMEMCACHEDCORE_LIBRARY NAMES ${libmemcached_lib} PATHS /usr/lib /usr/share /usr/lib64 /usr/local/lib /usr/local/lib64)
FIND_LIBRARY (LIBMEMCACHEDUTIL_LIBRARY NAMES ${libmemcachedUtil_lib} PATHS /usr/lib /usr/share /usr/lib64 /usr/local/lib /usr/local/lib64)
Copy link
Member

Choose a reason for hiding this comment

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

trivial: indentation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I seem to have an issue with eclipse. I have tried many times to fix but it is un-compliant to always use ws instead of a tab.

@jamienoss
Copy link
Collaborator Author

On every 'get' I have asserted whether the correct routine is being used for the type that was set. I don't believe this to be concrete for the cases that the data was set externally to this plugin. If we prefixed/appended to all keys, partition and normal, then we could ensure (more than reasonably) that the data was set from the plugin. Thus preventing the user from reading externally set data, or warning of such and allowing for this to revoked along with the type assertion.

@richardkchapman
Copy link
Member

I don't think that forcing the data to be set by this plugin if it is going to be read by this plugin is a good idea - seems like you are restricting users from doing something they might reasonably want to do for no real good reason.

@richardkchapman
Copy link
Member

@jamienoss Please rebase

@jamienoss jamienoss force-pushed the issue11522-support-memcached branch 2 times, most recently from 7336762 to 44123ad Compare September 18, 2014 11:14
@jamienoss
Copy link
Collaborator Author

@ghalliday updated.

" STRING MGetString(CONST VARSTRING servers, CONST VARSTRING key, CONST VARSTRING partitionKey = 'root') : cpp,action,context,entrypoint='MGetStr'; \n"
" VARSTRING MGetVarstring(CONST VARSTRING servers, CONST VARSTRING key, CONST VARSTRING partitionKey = 'root') : cpp,action,context,entrypoint='MGetVarStr'; \n"
" UNICODE MGetUnicode(CONST VARSTRING servers, CONST VARSTRING key, CONST VARSTRING partitionKey = 'root') : cpp,action,context,entrypoint='MGetUChar'; \n"
" VARUNICODE MGetVarunicode(CONST VARSTRING servers, CONST VARSTRING key, CONST VARSTRING partitionKey = 'root') : cpp,action,context,entrypoint='MGetVarUChar'; \n"
Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't bother with this.

@jamienoss
Copy link
Collaborator Author

@ghalliday back to you. I didn't change the allServerCheck, see corresponding message.

############################################################################## */

export memcached := SERVICE : plugin('memcached')
BOOLEAN MSetUnicode(CONST VARSTRING servers, CONST VARSTRING key, CONST UNICODE value, CONST VARSTRING partitionKey = 'root', UNSIGNED4 expire = 0) : cpp,action,context,entrypoint='MSet';
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if these should remove the M prefix. The scope is already qualified by memcached.

The reason for bringing it up is that if we also provide a redis implementation it would make sense for the functions to be called the same - assuming they can have the same prototypes.

@ghalliday
Copy link
Member

@jamienoss Definitely looking much closer - a few minor comments, and one problem with ctx.

There are two outstanding questions.

  • What to do about servers that are down. We need to discuss with Richard. I don't have strong views.
  • Are we specifying a canonical format (e.g., utf8) for strings, or storing them in their natural format for each call. I think the current (natural) scheme works well, but it is worth deciding explicitly and including it in the documentation. (We could convert between stored and requested formats, but that can be a separate request.)

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 24, 2014
Previously: hpcc-systems#6368

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 24, 2014
Previously: hpcc-systems#6368

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 24, 2014
Previously: hpcc-systems#6368

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 24, 2014
Previously: hpcc-systems#6368

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 24, 2014
Previously: hpcc-systems#6368

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 24, 2014
Previously: hpcc-systems#6368

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
@jamienoss
Copy link
Collaborator Author

Moved to #6686

@jamienoss jamienoss closed this Nov 24, 2014
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 24, 2014
Previously: hpcc-systems#6368

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 24, 2014
Previously: hpcc-systems#6368

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 24, 2014
Previously: hpcc-systems#6368

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 25, 2014
Previously: hpcc-systems#6368 & hpcc-systems#6686

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 25, 2014
Previously: hpcc-systems#6368 & hpcc-systems#6686

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 25, 2014
Previously: hpcc-systems#6368 & hpcc-systems#6686

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 26, 2014
Previously: hpcc-systems#6368 & hpcc-systems#6686

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
jamienoss added a commit to jamienoss/HPCC-Platform that referenced this pull request Nov 28, 2014
Previously: hpcc-systems#6368 & hpcc-systems#6686

Signed-off-by: jamienoss <james.noss@lexisnexis.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants