[GitHub] ctubbsii commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor
ctubbsii commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor URL: https://github.com/apache/accumulo/pull/289#issuecomment-345899199 The current branch is still internally consistent. The change in the master branch would only matter if one of those classes were somehow getting on the class path, which they shouldn't be. But, it is possible that the changes in master are more compatible with the mocking mechanism. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor
glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor URL: https://github.com/apache/accumulo/pull/289#issuecomment-345890219 ~~Ah, I got this to break locally with the right mvn incantation (was running the sunny day tests instead of the mvn clean verify -DskipITs that travis is running). I'll see if I can get a workaround over Thanksgiving holiday; otherwise I can add an Ignore annotation or gut the PowerMock test.~~ I noticed that my branch is pretty far behind master at this point and it looks like the real issue is that the Table.ID inner class actually changed (ACCUMULO-4681). Previously it has a public constructor and now it appears to have a private one hence the test failure. Those changes looked like they were merged a couple of weeks after my original PR was posted. I'll take a look over Thanksgiving and see if I can get this working again. As it stands it should not be merged. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor
glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor URL: https://github.com/apache/accumulo/pull/289#issuecomment-345890219 Ah, I got this to break locally with the right mvn incantation (was running the sunny day tests instead of the mvn clean verify -DskipITs that travis is running). I'll see if I can get a workaround over Thanksgiving holiday; otherwise I can add an Ignore annotation or gut the PowerMock test. I also noticed that my branch is pretty far behind master at this point and it looks like the real issue is that the Table.ID inner class actually changed. Previously it has a public constructor and now it appears to have a private one hence the test failure. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor
glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor URL: https://github.com/apache/accumulo/pull/289#issuecomment-345890219 Ah, I got this to break locally with the right mvn incantation (was running the sunny day tests instead of the mvn clean verify -DskipITs that travis is running). I'll see if I can get a workaround over Thanksgiving holiday; otherwise I can add an Ignore annotation or gut the PowerMock test. I also noticed that my branch is pretty far behind master at this point and it looks like the **real issue** is that the Table.ID inner class actually changed. Previously it has a public constructor and now it appears to have a private one hence the test failure. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor
glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor URL: https://github.com/apache/accumulo/pull/289#issuecomment-345890219 Ah, I got this to break locally with the right mvn incantation (was running the sunny day tests instead of the mvn clean verify -DskipITs that travis is running). I'll see if I can get a workaround over Thanksgiving holiday; otherwise I can add an Ignore annotation or gut the PowerMock test. I also noticed that my branch is pretty far behind master at this point and it looks like the real issue is that the Table.ID inner class actually changed. Previously it has a public constructor and now it appears to have a private one hence the test failure. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor
glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor URL: https://github.com/apache/accumulo/pull/289#issuecomment-345890219 Ah, I got this to break locally with the right mvn incantation (was running the sunny day tests instead of the mvn clean verify -DskipITs that travis is running). I'll see if I can get a workaround over Thanksgiving holiday; otherwise I can add an Ignore annotation or gut the PowerMock test. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[jira] [Updated] (ACCUMULO-4744) Using RFile API with cache and multiple files hides data
[
https://issues.apache.org/jira/browse/ACCUMULO-4744?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
ASF GitHub Bot updated ACCUMULO-4744:
-
Labels: pull-request-available (was: )
> Using RFile API with cache and multiple files hides data
>
>
> Key: ACCUMULO-4744
> URL: https://issues.apache.org/jira/browse/ACCUMULO-4744
> Project: Accumulo
> Issue Type: Bug
>Affects Versions: 1.8.0, 1.8.1
>Reporter: Keith Turner
>Priority: Critical
> Labels: pull-request-available
> Fix For: 1.8.2
>
>
> Noticed this bug in source code while working on ACCUMULO-4641. When using
> the RFile API introduced in 1.8 to read from multiple files with cache
> enabled, not all data may be seen. This happens because internally the code
> gives all input sources the same cache id. Therefore index and data blocks
> from multiple files collide in the cache.
> This bug does not happen when reading data through tserver, only the RFile
> API.
> {code:java}
> Scanner scanner =
>RFile.newScanner()
>.from(file1, file2, file3) //multiple input files
>.withFileSystem(localFs)
>.withIndexCache(100) //enabled cache
>.withDataCache(1000) //enabled cache
>.build();
> {code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[GitHub] keith-turner opened a new pull request #324: ACCUMULO-4744 Fixed RFile API scanner bug
keith-turner opened a new pull request #324: ACCUMULO-4744 Fixed RFile API scanner bug URL: https://github.com/apache/accumulo/pull/324 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[jira] [Created] (ACCUMULO-4744) Using RFile API with cache and multiple files hides data
Keith Turner created ACCUMULO-4744:
--
Summary: Using RFile API with cache and multiple files hides data
Key: ACCUMULO-4744
URL: https://issues.apache.org/jira/browse/ACCUMULO-4744
Project: Accumulo
Issue Type: Bug
Affects Versions: 1.8.1, 1.8.0
Reporter: Keith Turner
Priority: Critical
Fix For: 1.8.2
Noticed this bug in source code while working on ACCUMULO-4641. When using the
RFile API introduced in 1.8 to read from multiple files with cache enabled, not
all data may be seen. This happens because internally the code gives all input
sources the same cache id. Therefore index and data blocks from multiple files
collide in the cache.
This bug does not happen when reading data through tserver, only the RFile API.
{code:java}
Scanner scanner =
RFile.newScanner()
.from(file1, file2, file3) //multiple input files
.withFileSystem(localFs)
.withIndexCache(100) //enabled cache
.withDataCache(1000) //enabled cache
.build();
{code}
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
[GitHub] ctubbsii commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor
ctubbsii commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor URL: https://github.com/apache/accumulo/pull/289#issuecomment-345812832 Hmm, I see. Well, maybe we can skip this test on Travis only? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor
glitch commented on issue #289: ACCUMULO-4677 Sanitizing PathParam values in REST-based Monitor URL: https://github.com/apache/accumulo/pull/289#issuecomment-345810287 I needed PowerMock because EasyMock couldn't mock static methods (i.e. in the WebViewsTest needed to mock the Monitor.getContext() method to return a different mocked/stub context). I can get rid of those specific tests using Powermock but I don't have anything to replace them with. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[jira] [Created] (ACCUMULO-4743) Use tserver prefix for Cache config instead of general custom
Keith Turner created ACCUMULO-4743:
--
Summary: Use tserver prefix for Cache config instead of general
custom
Key: ACCUMULO-4743
URL: https://issues.apache.org/jira/browse/ACCUMULO-4743
Project: Accumulo
Issue Type: Bug
Reporter: Keith Turner
Fix For: 2.0.0
In ACCUMULO-4463 caching implementations were made configurable. At the time
config for a cache was placed under the {{general.custom.cache}} prefix. The
idea was that a cache impl would be passed in a AccumuloConfiguration object
and it could grab whatever it wanted from {{general.custom.cache}}.
In ACCUMULO-4644 the cache API was changed to not use AccumuloConfiguration
because that type is not in the public API. This removed the reason for using
general.custom in the first place.
When trying to implement an external cache I discovered that I can not set
general.custom props in zookeeper. However the cache class is a tserver
prefixed property can can be set in zookeeper. For the
[accumulo-ohc|https://github.com/keith-turner/accumulo-ohc] I wanted to do the
following in the shell but could not set the general props.
{noformat}
config -s general.custom.cache.ohc.data.on-heap.maximumWeight=10
config -s general.custom.cache.ohc.data.off-heap.capacity=100
config -s tserver.cache.manager.class=accumulo.ohc.OhcCacheManager
{noformat}
I think it would be nice to change cache config prefix from
{{general.custom.cache..}} to
{{tserver.cache.config..}}. Doing this would enable setting
up a cache in the shell like the following.
{noformat}
config -s tserver.cache.config.data.on-heap.maximumWeight=10
config -s tserver.cache.config.data.off-heap.capacity=100
config -s tserver.cache.manager.class=accumulo.ohc.OhcCacheManager
{noformat}
Part of the code for this prefix is at [BlockCacheManager.java line
33|https://github.com/apache/accumulo/blob/d877a2df2943e48d70d99b96616844d0dff9a501/core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCacheManager.java#L33]
--
This message was sent by Atlassian JIRA
(v6.4.14#64029)
