You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pirk.apache.org by jkovba <gi...@git.apache.org> on 2016/08/02 00:54:37 UTC
[GitHub] incubator-pirk pull request #43: PIRK-29
GitHub user jkovba opened a pull request:
https://github.com/apache/incubator-pirk/pull/43
PIRK-29
Implemented a check to ensure the queryID (formerly named queryName) in the Querier matches the queryID in the Response object.
Added a timestamp to the queryID for uniqueness.
Renamed some fields/methods for uniformity.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jkovba/incubator-pirk PIRK-29
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-pirk/pull/43.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #43
----
commit 22e11ab4701a79ed8e6095f311b120cd11b76413
Author: jkovba <jk...@ltsadmins-mbp.home>
Date: 2016-07-29T22:48:10Z
Renamed getPirWatchlist() to getQueryInfo() in Querier.java to match the
method name in Response.java
commit 0ee6a93acc199ab9b143f541b4d077b2a3546eaf
Author: jkovba <jk...@ltsadmins-mbp.home>
Date: 2016-07-30T11:11:31Z
Changed all occurrences of queryName to queryID in Querier.java
commit c7210071e991828b5dfd41511e60f87242eef409
Author: jkovba <jk...@ltsadmins-mbp.home>
Date: 2016-07-30T11:14:53Z
Changed all occurrences of queryName to queryID in QuerierDriver.java
and QuerierDriveCLI.java
commit bc1b2809e948290e970c63eec49e3fcabe69891a
Author: jkovba <jk...@ltsadmins-mbp.home>
Date: 2016-07-30T12:00:24Z
Added check to ensure queryID in Querier file matches the one in the
Response
commit 417c492dc79c431e079baffb9a4b1178fe28e31e
Author: jkovba <jk...@ltsadmins-mbp.home>
Date: 2016-07-30T12:12:51Z
Added unique date/time string to QueryID field in QuerierDriverCLI.java
commit a7435b684bd57eb47b915bd37987912518b96181
Author: jkovba <jk...@ltsadmins-mbp.home>
Date: 2016-08-02T00:30:27Z
Changed getQueryName to getQueryID in a few places.
commit 60e8e0599f969d146bdb580e11ab63b8188bb6ce
Author: Joseph Kovba <jk...@ltsadmins-mbp.home>
Date: 2016-08-02T00:50:18Z
Added a change
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pirk pull request #43: PIRK-29
Posted by smarthi <gi...@git.apache.org>.
Github user smarthi commented on a diff in the pull request:
https://github.com/apache/incubator-pirk/pull/43#discussion_r73238652
--- Diff: src/main/java/org/apache/pirk/querier/wideskies/QuerierDriverCLI.java ---
@@ -239,12 +242,19 @@ private boolean parseOptions()
}
SystemConfiguration.setProperty(CERTAINTY, getOptionValue(CERTAINTY));
- if (!hasOption(QUERYNAME))
+ if (!hasOption(QUERYID))
{
- logger.info("Must have the option " + QUERYNAME);
+ logger.info("Must have the option " + QUERYID);
return false;
}
- SystemConfiguration.setProperty(QUERYNAME, getOptionValue(QUERYNAME));
+
+ // create a formatted date/time string
+ Date date = new Date();
+ SimpleDateFormat sdf = new SimpleDateFormat("MM.dd.yyyy_hh.mm.ss");
+ String formattedDate = sdf.format(date);
--- End diff --
I believe we decided to go with Java 8 for Pirk and the POM has been updated to reflect that. The link u point to needs to be updated too. Pirk is officially now on Java 8. Suggest u rebase ur code with the present master and update this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pirk pull request #43: PIRK-29
Posted by ellisonanne <gi...@git.apache.org>.
Github user ellisonanne commented on a diff in the pull request:
https://github.com/apache/incubator-pirk/pull/43#discussion_r73314683
--- Diff: src/main/java/org/apache/pirk/querier/wideskies/QuerierDriverCLI.java ---
@@ -239,12 +242,19 @@ private boolean parseOptions()
}
SystemConfiguration.setProperty(CERTAINTY, getOptionValue(CERTAINTY));
- if (!hasOption(QUERYNAME))
+ if (!hasOption(QUERYID))
{
- logger.info("Must have the option " + QUERYNAME);
+ logger.info("Must have the option " + QUERYID);
return false;
}
- SystemConfiguration.setProperty(QUERYNAME, getOptionValue(QUERYNAME));
+
+ // create a formatted date/time string
+ Date date = new Date();
+ SimpleDateFormat sdf = new SimpleDateFormat("MM.dd.yyyy_hh.mm.ss");
+ String formattedDate = sdf.format(date);
--- End diff --
Correct - we just went to 1.8 - I will update the website.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pirk pull request #43: PIRK-29
Posted by ellisonanne <gi...@git.apache.org>.
Github user ellisonanne commented on a diff in the pull request:
https://github.com/apache/incubator-pirk/pull/43#discussion_r73229816
--- Diff: src/main/java/org/apache/pirk/querier/wideskies/QuerierDriver.java ---
@@ -91,7 +91,7 @@ public static void main(String... args) throws IOException, InterruptedException
int dataPartitionBitSize = 0;
int paillierBitSize = 0;
int certainty = 0;
- String queryName = null;
--- End diff --
The queryName corresponds to the schemaName in the query-schema.xsd file *not* to the user-given identifier for the query -- the identifier for this query is actually the queryNum. Notice that queryNum is currently a double -- if we want to start appending dates or whatever else to it, we need to change its type to String throughout.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
Re: QueryInfo.queryNum
Posted by Walter Ray-Dulany <ra...@gmail.com>.
+1 sounds good
On Aug 10, 2016 12:25, "Ellison Anne Williams" <ea...@gmail.com>
wrote:
> +1 Combining sounds good to me
>
> On Wed, Aug 10, 2016 at 11:58 AM, Suneel Marthi <su...@gmail.com>
> wrote:
>
> > On Wed, Aug 10, 2016 at 11:51 AM, Tim Ellison <t....@gmail.com>
> > wrote:
> >
> > > On 10/08/16 15:34, Ellison Anne Williams wrote:
> > > > First, there are some wacko comments that I made in that pull request
> > > that
> > > > I tried to delete but don't appear to have quite cleared (in my
> haste,
> > I
> > > > started making comments before going through all of the changes -
> turns
> > > out
> > > > I was looking at the changes in the wrong way and then tried to
> > delete...
> > > > and failed.. and then let confusion ensue... won't do that again...)
> > >
> > > :-)
> > >
> > > > The queryID (originally the queryName in the code) is the
> user-assigned
> > > ID
> > > > of the query. Currently, the user can embed whatever info they would
> > like
> > > > in the ID -- Pirk doesn't 'do' anything to it other than maintain it.
> > > Thus,
> > > > I think that changing it to a UUID object makes sense. In that case,
> we
> > > > will be using the toString and fromString methods of the UUID class
> to
> > > > write/parse the ID.
> > >
> > > There is currently a queryNum (double) and a queryName (String) that
> > > both seem to be simply used to identify the query for the benefit of
> the
> > > end user. How about I combine them into a single UUID identifier?
> > >
> > > +1
> >
> >
> > > Regards,
> > > Tim
> > >
> >
>
Re: QueryInfo.queryNum
Posted by Ellison Anne Williams <ea...@gmail.com>.
+1 Combining sounds good to me
On Wed, Aug 10, 2016 at 11:58 AM, Suneel Marthi <su...@gmail.com>
wrote:
> On Wed, Aug 10, 2016 at 11:51 AM, Tim Ellison <t....@gmail.com>
> wrote:
>
> > On 10/08/16 15:34, Ellison Anne Williams wrote:
> > > First, there are some wacko comments that I made in that pull request
> > that
> > > I tried to delete but don't appear to have quite cleared (in my haste,
> I
> > > started making comments before going through all of the changes - turns
> > out
> > > I was looking at the changes in the wrong way and then tried to
> delete...
> > > and failed.. and then let confusion ensue... won't do that again...)
> >
> > :-)
> >
> > > The queryID (originally the queryName in the code) is the user-assigned
> > ID
> > > of the query. Currently, the user can embed whatever info they would
> like
> > > in the ID -- Pirk doesn't 'do' anything to it other than maintain it.
> > Thus,
> > > I think that changing it to a UUID object makes sense. In that case, we
> > > will be using the toString and fromString methods of the UUID class to
> > > write/parse the ID.
> >
> > There is currently a queryNum (double) and a queryName (String) that
> > both seem to be simply used to identify the query for the benefit of the
> > end user. How about I combine them into a single UUID identifier?
> >
> > +1
>
>
> > Regards,
> > Tim
> >
>
Re: QueryInfo.queryNum
Posted by Suneel Marthi <su...@gmail.com>.
On Wed, Aug 10, 2016 at 11:51 AM, Tim Ellison <t....@gmail.com> wrote:
> On 10/08/16 15:34, Ellison Anne Williams wrote:
> > First, there are some wacko comments that I made in that pull request
> that
> > I tried to delete but don't appear to have quite cleared (in my haste, I
> > started making comments before going through all of the changes - turns
> out
> > I was looking at the changes in the wrong way and then tried to delete...
> > and failed.. and then let confusion ensue... won't do that again...)
>
> :-)
>
> > The queryID (originally the queryName in the code) is the user-assigned
> ID
> > of the query. Currently, the user can embed whatever info they would like
> > in the ID -- Pirk doesn't 'do' anything to it other than maintain it.
> Thus,
> > I think that changing it to a UUID object makes sense. In that case, we
> > will be using the toString and fromString methods of the UUID class to
> > write/parse the ID.
>
> There is currently a queryNum (double) and a queryName (String) that
> both seem to be simply used to identify the query for the benefit of the
> end user. How about I combine them into a single UUID identifier?
>
> +1
> Regards,
> Tim
>
Re: QueryInfo.queryNum
Posted by Tim Ellison <t....@gmail.com>.
On 10/08/16 15:34, Ellison Anne Williams wrote:
> First, there are some wacko comments that I made in that pull request that
> I tried to delete but don't appear to have quite cleared (in my haste, I
> started making comments before going through all of the changes - turns out
> I was looking at the changes in the wrong way and then tried to delete...
> and failed.. and then let confusion ensue... won't do that again...)
:-)
> The queryID (originally the queryName in the code) is the user-assigned ID
> of the query. Currently, the user can embed whatever info they would like
> in the ID -- Pirk doesn't 'do' anything to it other than maintain it. Thus,
> I think that changing it to a UUID object makes sense. In that case, we
> will be using the toString and fromString methods of the UUID class to
> write/parse the ID.
There is currently a queryNum (double) and a queryName (String) that
both seem to be simply used to identify the query for the benefit of the
end user. How about I combine them into a single UUID identifier?
Regards,
Tim
Re: QueryInfo.queryNum (was: Re: [GitHub] incubator-pirk pull request
#43: PIRK-29)
Posted by Ellison Anne Williams <ea...@gmail.com>.
First, there are some wacko comments that I made in that pull request that
I tried to delete but don't appear to have quite cleared (in my haste, I
started making comments before going through all of the changes - turns out
I was looking at the changes in the wrong way and then tried to delete...
and failed.. and then let confusion ensue... won't do that again...)
The queryID (originally the queryName in the code) is the user-assigned ID
of the query. Currently, the user can embed whatever info they would like
in the ID -- Pirk doesn't 'do' anything to it other than maintain it. Thus,
I think that changing it to a UUID object makes sense. In that case, we
will be using the toString and fromString methods of the UUID class to
write/parse the ID.
On Wed, Aug 10, 2016 at 9:10 AM, Tim Ellison <t....@gmail.com> wrote:
> On 02/08/16 21:23, ellisonanne wrote:
> > Github user ellisonanne commented on a diff in the pull request:
> >
> > https://github.com/apache/incubator-pirk/pull/43#discussion_r73229948
> >
> > --- Diff:
> > src/main/java/org/apache/pirk/querier/wideskies/QuerierDriverCLI.java
> > --- @@ -55,7 +58,7 @@ public static final String PAILLIERBITSIZE =
> > "paillierBitSize"; public static final String BITSET = "bitSet";
> > public static final String CERTAINTY = "certainty"; - public static
> > final String QUERYNAME = "queryName"; + public static final String
> > QUERYID = "queryID"; --- End diff --
> >
> > See previous comment on the QuerierDriver:
> >
> > The queryName corresponds to the schemaName in the query-schema.xsd
> > file not to the user-given identifier for the query -- the identifier
> > for this query is actually the queryNum. Notice that queryNum is
> > currently a double -- if we want to start appending dates or whatever
> > else to it, we need to change its type to String throughout.
>
> I had been wondering why this was a double -- seems strange.
>
> I was considering changing it to be a UUID, what do you think?
>
> While it may be ok given Pirk's policy on trust to use an RFC 4122
> type 1 UUID, which would reveal to the responder the machine IP address
> and timestamp in the query, that would make it more difficult for higher
> level applications to preserve the anonymity of a querier, so I'm
> inclined to opt for the slightly more time consuming type 4 (random) UUID.
>
> I think the performance difference will be lost in the noise.
>
> Thoughts on this too?
>
> Regards,
> Tim
>
QueryInfo.queryNum (was: Re: [GitHub] incubator-pirk pull request
#43: PIRK-29)
Posted by Tim Ellison <t....@gmail.com>.
On 02/08/16 21:23, ellisonanne wrote:
> Github user ellisonanne commented on a diff in the pull request:
>
> https://github.com/apache/incubator-pirk/pull/43#discussion_r73229948
>
> --- Diff:
> src/main/java/org/apache/pirk/querier/wideskies/QuerierDriverCLI.java
> --- @@ -55,7 +58,7 @@ public static final String PAILLIERBITSIZE =
> "paillierBitSize"; public static final String BITSET = "bitSet";
> public static final String CERTAINTY = "certainty"; - public static
> final String QUERYNAME = "queryName"; + public static final String
> QUERYID = "queryID"; --- End diff --
>
> See previous comment on the QuerierDriver:
>
> The queryName corresponds to the schemaName in the query-schema.xsd
> file not to the user-given identifier for the query -- the identifier
> for this query is actually the queryNum. Notice that queryNum is
> currently a double -- if we want to start appending dates or whatever
> else to it, we need to change its type to String throughout.
I had been wondering why this was a double -- seems strange.
I was considering changing it to be a UUID, what do you think?
While it may be ok given Pirk's policy on trust to use an RFC 4122
type 1 UUID, which would reveal to the responder the machine IP address
and timestamp in the query, that would make it more difficult for higher
level applications to preserve the anonymity of a querier, so I'm
inclined to opt for the slightly more time consuming type 4 (random) UUID.
I think the performance difference will be lost in the noise.
Thoughts on this too?
Regards,
Tim
[GitHub] incubator-pirk pull request #43: PIRK-29
Posted by ellisonanne <gi...@git.apache.org>.
Github user ellisonanne commented on a diff in the pull request:
https://github.com/apache/incubator-pirk/pull/43#discussion_r73229948
--- Diff: src/main/java/org/apache/pirk/querier/wideskies/QuerierDriverCLI.java ---
@@ -55,7 +58,7 @@
public static final String PAILLIERBITSIZE = "paillierBitSize";
public static final String BITSET = "bitSet";
public static final String CERTAINTY = "certainty";
- public static final String QUERYNAME = "queryName";
+ public static final String QUERYID = "queryID";
--- End diff --
See previous comment on the QuerierDriver:
The queryName corresponds to the schemaName in the query-schema.xsd file not to the user-given identifier for the query -- the identifier for this query is actually the queryNum. Notice that queryNum is currently a double -- if we want to start appending dates or whatever else to it, we need to change its type to String throughout.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pirk issue #43: PIRK-29
Posted by ellisonanne <gi...@git.apache.org>.
Github user ellisonanne commented on the issue:
https://github.com/apache/incubator-pirk/pull/43
I'm going to close this PR as the decision to make the query identifier to a UUID supersedes these changes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pirk pull request #43: PIRK-29
Posted by DarinJ <gi...@git.apache.org>.
Github user DarinJ commented on a diff in the pull request:
https://github.com/apache/incubator-pirk/pull/43#discussion_r73237838
--- Diff: src/main/java/org/apache/pirk/querier/wideskies/QuerierDriverCLI.java ---
@@ -239,12 +242,19 @@ private boolean parseOptions()
}
SystemConfiguration.setProperty(CERTAINTY, getOptionValue(CERTAINTY));
- if (!hasOption(QUERYNAME))
+ if (!hasOption(QUERYID))
{
- logger.info("Must have the option " + QUERYNAME);
+ logger.info("Must have the option " + QUERYID);
return false;
}
- SystemConfiguration.setProperty(QUERYNAME, getOptionValue(QUERYNAME));
+
+ // create a formatted date/time string
+ Date date = new Date();
+ SimpleDateFormat sdf = new SimpleDateFormat("MM.dd.yyyy_hh.mm.ss");
+ String formattedDate = sdf.format(date);
--- End diff --
According to: https://pirk.incubator.apache.org/for_users, the required JDK is 1.7. I'm pro 1.8, but that's not the subject of this PR. Was the decision made to drop 1.7 support?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pirk pull request #43: PIRK-29
Posted by ellisonanne <gi...@git.apache.org>.
Github user ellisonanne commented on a diff in the pull request:
https://github.com/apache/incubator-pirk/pull/43#discussion_r73230035
--- Diff: src/main/java/org/apache/pirk/query/wideskies/QueryInfo.java ---
@@ -43,7 +43,7 @@
private String queryType = null; // QueryType string const
- private String queryName = null; // Name of query
+ private String queryID = null; // Unique query ID
--- End diff --
See previous notes above
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pirk issue #43: PIRK-29
Posted by ellisonanne <gi...@git.apache.org>.
Github user ellisonanne commented on the issue:
https://github.com/apache/incubator-pirk/pull/43
I think that these changes look good.
Joe - Can you please merge this PR branch with the current master (your base version of master is rather 'old')? Then, it will be easy to accept and merge the pull request. Thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pirk pull request #43: PIRK-29
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-pirk/pull/43
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pirk pull request #43: PIRK-29
Posted by smarthi <gi...@git.apache.org>.
Github user smarthi commented on a diff in the pull request:
https://github.com/apache/incubator-pirk/pull/43#discussion_r73080214
--- Diff: src/main/java/org/apache/pirk/querier/wideskies/QuerierDriverCLI.java ---
@@ -239,12 +242,19 @@ private boolean parseOptions()
}
SystemConfiguration.setProperty(CERTAINTY, getOptionValue(CERTAINTY));
- if (!hasOption(QUERYNAME))
+ if (!hasOption(QUERYID))
{
- logger.info("Must have the option " + QUERYNAME);
+ logger.info("Must have the option " + QUERYID);
return false;
}
- SystemConfiguration.setProperty(QUERYNAME, getOptionValue(QUERYNAME));
+
+ // create a formatted date/time string
+ Date date = new Date();
+ SimpleDateFormat sdf = new SimpleDateFormat("MM.dd.yyyy_hh.mm.ss");
+ String formattedDate = sdf.format(date);
--- End diff --
Could we use Java 8 LocalDateTime and DateTimFormatter APIs instead ?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-pirk issue #43: PIRK-29
Posted by ellisonanne <gi...@git.apache.org>.
Github user ellisonanne commented on the issue:
https://github.com/apache/incubator-pirk/pull/43
...Should also add that we can add in the line to perform the query ID check in the Querier (as in the corresponding JIRA issue) to a new PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---