The entire code from the initial commit

The entire code from initial commit too the added support...(excluded)

[robert.erneborg, 2015-10-06 14:04:08]

This review contains the entire code from the first commit up until the first
patch published to code review, i.e. "support for creating new queues in rabbit
mq" commit #990a3ac4babcd16bd46a54989b44eb1910ac7ff1

[kirill.sc, 2015-10-07 09:26:10]

Generally good looking code, my high level comments are subject to my correct
interpretation of intended use. 

I think you might benefit from more comments on class descriptions and scope of
use, or having a documentation that cover that, however this is rather
subjective. 

Also1: Consider to rename "The entire code from the initial commit" to more
meaningful name "(Team-5) Initial Core logic...."

Also2: Please close/remove your previous partial code reviews if people are not
looking into them.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/config/AppInitializer.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/config/AppInitializer.java:23:
System.out.println("MainWebAppInitializer.onStartup()");
consider to use logger

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/config/AppInitializer.java:46: throw new
IllegalStateException("'appServlet' could not be mapped to '/' due " + "to an
existing mapping. This is a known issue under Tomcat versions " + "<= 7.0.14;
see https://issues.apache.org/bugzilla/show_bug.cgi?id=51278");
consider to use proper string formatting

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/config/ServerConfig.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/config/ServerConfig.java:33: //    protected
final String helloWorldQueueName = "new.test.queue";
remove commented out code

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/config/WebMVCConfig.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/config/WebMVCConfig.java:29: import
se.kth.crypt.server.model.GeoEntry;
never used

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/DeleteController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/DeleteController.java:34:
database.remove(entry.getKey(), value);
where do you initiate database object, and do you need to check if null?

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/DeleteController.java:36: return
"Value deleted for key: " + entry.getKey();
What is the intended use of such return statement? Would it make sense to log
this message instead and return success failure instead?

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/IndexController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/IndexController.java:18: class
IndexController
What does IndexController suppose to do? Single function here has nothing to do
with "Index(ing)"

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/IndexController.java:40: return
"Server is upppppp";
Server probably is __not__ upppppp

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/ReadController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/ReadController.java:32: if(key ==
null || key.length() < 1){
perhaps key.isEmpty() more readable

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/ReadController.java:38:
HashMap<String, String> values = new HashMap<String, String>();
move "new" after if statement, no need to create hash map if we throwing an
exception

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/UpdateController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/UpdateController.java:35: public
String updateRequest(@RequestBody GeoEntry entry) throws
IllegalArgumentException, IllegalAccessException {
I think here you can/should just reuse writeRequest

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/WriteController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/WriteController.java:42: if
(entry.getValues().size() != 1){
isEmpty()?
or
This table __should__ have one field. ?

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/WriteController.java:50: value =
entry.getValues().get(c);
What is the purpose of this loop? Are you trying to get the last key in a
hashmap? (I dont think it is actually sorted) (you can do this without a loop)
Also this goes against values.put("field0", value); in your read function.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/WriteController.java:59: return
"Value added for key: " + entry.getKey();
Again, why such return, is it some YCSB requirement?

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/core/RmqReceiverHandler.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/core/RmqReceiverHandler.java:25:
ConcurrentHashMap<String, String> test;
what is this name?

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/core/RmqReceiverHandler.java:45: if
(test.isEmpty())
Doesn't make sense to me. If it is a test code, remove it from here

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/model/GeoEntry.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/model/GeoEntry.java:8: public class GeoEntry {
Is this is a single entry in your database, i.e. a row? 
If I recall correctly, we have discussed to use only a first (out of 9 or 10)
string from YCSB hash map?
why do you have HashMap as values?

[robert.erneborg, 2015-10-07 15:00:27]

I have corrected and changed everything discussed in this review except the
comments I had questions about. You can see the updated code review issue at
https://codereview.appspot.com/261680043/.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/config/AppInitializer.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/config/AppInitializer.java:23:
System.out.println("MainWebAppInitializer.onStartup()");
On 2015/10/07 09:26:09, kirill.sc wrote:
> consider to use logger

We had some problems with the logger but solved it today so I will add log
output throughout the code

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/config/AppInitializer.java:46: throw new
IllegalStateException("'appServlet' could not be mapped to '/' due " + "to an
existing mapping. This is a known issue under Tomcat versions " + "<= 7.0.14;
see https://issues.apache.org/bugzilla/show_bug.cgi?id=51278");
On 2015/10/07 09:26:09, kirill.sc wrote:
> consider to use proper string formatting

Done.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/config/ServerConfig.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/config/ServerConfig.java:33: //    protected
final String helloWorldQueueName = "new.test.queue";
On 2015/10/07 09:26:09, kirill.sc wrote:
> remove commented out code

Done.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/config/WebMVCConfig.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/config/WebMVCConfig.java:29: import
se.kth.crypt.server.model.GeoEntry;
On 2015/10/07 09:26:10, kirill.sc wrote:
> never used

Done.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/DeleteController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/DeleteController.java:34:
database.remove(entry.getKey(), value);
On 2015/10/07 09:26:10, kirill.sc wrote:
> where do you initiate database object, and do you need to check if null?

I declare it in serverconfig.java and since it is declared as a bean the spring
container will initialize and handle the variable. We can access it by using
@Autowire. This lets spring know that we are trying to access a bean for the
current context.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/DeleteController.java:36: return
"Value deleted for key: " + entry.getKey();
On 2015/10/07 09:26:10, kirill.sc wrote:
> What is the intended use of such return statement? Would it make sense to log
> this message instead and return success failure instead?

We don't really need to return more than a HTTP ok (which we do) but for
debugging reasons (in wireshark) we included a return string that will be added
to the response body

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/IndexController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/IndexController.java:18: class
IndexController
On 2015/10/07 09:26:10, kirill.sc wrote:
> What does IndexController suppose to do? Single function here has nothing to
do
> with "Index(ing)"

regarding the name it is temporary and the reason behind it is that it
corresponds to the single / url which in a regular http server returns
index.html. However io the future I plan to reduce the number of controller
classes to one single with all the methods from the current controllers.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/IndexController.java:40: return
"Server is upppppp";
On 2015/10/07 09:26:10, kirill.sc wrote:
> Server probably is __not__ upppppp

the message queue was part of testing in an earlier stage and should be removed.
If server is up is returned it actually means that the http server is up. So to
also include the first comment here we wanted that the single / url would tell
us information about the server status for debugging. We will probably add more
statuses here.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/ReadController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/ReadController.java:32: if(key ==
null || key.length() < 1){
On 2015/10/07 09:26:10, kirill.sc wrote:
> perhaps key.isEmpty() more readable

great suggestion, never used that method actually but it makes sense!

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/ReadController.java:38:
HashMap<String, String> values = new HashMap<String, String>();
On 2015/10/07 09:26:10, kirill.sc wrote:
> move "new" after if statement, no need to create hash map if we throwing an
> exception

not using hashmap any more

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/UpdateController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/UpdateController.java:35: public
String updateRequest(@RequestBody GeoEntry entry) throws
IllegalArgumentException, IllegalAccessException {
On 2015/10/07 09:26:10, kirill.sc wrote:
> I think here you can/should just reuse writeRequest

yo mean that the client instead should use write when (if) performing update?

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/WriteController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/WriteController.java:42: if
(entry.getValues().size() != 1){
On 2015/10/07 09:26:10, kirill.sc wrote:
> isEmpty()?
> or
> This table __should__ have one field. ?

We have since this commit changed the structure of GeoEntry and we're not using
hashmap any more! So this code has been modified and corrected

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/WriteController.java:59: return
"Value added for key: " + entry.getKey();
On 2015/10/07 09:26:10, kirill.sc wrote:
> Again, why such return, is it some YCSB requirement?

We want the client to know that the message was received and the parameters sent
was ok. do you think we shouln't let the client know if something was wrong with
the request?

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/core/RmqReceiverHandler.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/core/RmqReceiverHandler.java:25:
ConcurrentHashMap<String, String> test;
On 2015/10/07 09:26:10, kirill.sc wrote:
> what is this name?

should be removed, used for testing :P

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/core/RmqReceiverHandler.java:45: if
(test.isEmpty())
On 2015/10/07 09:26:10, kirill.sc wrote:
> Doesn't make sense to me. If it is a test code, remove it from here

will do!

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/model/GeoEntry.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/model/GeoEntry.java:8: public class GeoEntry {
On 2015/10/07 09:26:10, kirill.sc wrote:
> Is this is a single entry in your database, i.e. a row? 
> If I recall correctly, we have discussed to use only a first (out of 9 or 10)
> string from YCSB hash map?
> why do you have HashMap as values?

we have changed this now!

[kirill.sc, 2015-10-07 17:18:14]

ok, great.

Now, everyone else have a chance to look through the code and get some idea from
our conversation. Or similarly comment and ask questions about latests reviews.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/config/AppInitializer.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/config/AppInitializer.java:23:
System.out.println("MainWebAppInitializer.onStartup()");
On 2015/10/07 15:00:27, robert.erneborg wrote:
> On 2015/10/07 09:26:09, kirill.sc wrote:
> > consider to use logger
> 
> We had some problems with the logger but solved it today so I will add log
> output throughout the code

Acknowledged.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/DeleteController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/DeleteController.java:34:
database.remove(entry.getKey(), value);
On 2015/10/07 15:00:27, robert.erneborg wrote:
> On 2015/10/07 09:26:10, kirill.sc wrote:
> > where do you initiate database object, and do you need to check if null?
> 
> I declare it in serverconfig.java and since it is declared as a bean the
spring
> container will initialize and handle the variable. We can access it by using
> @Autowire. This lets spring know that we are trying to access a bean for the
> current context.

ok, then I presume it is safe to assume that database is initiated at this
stage.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/DeleteController.java:36: return
"Value deleted for key: " + entry.getKey();
On 2015/10/07 15:00:27, robert.erneborg wrote:
> On 2015/10/07 09:26:10, kirill.sc wrote:
> > What is the intended use of such return statement? Would it make sense to
log
> > this message instead and return success failure instead?
> 
> We don't really need to return more than a HTTP ok (which we do) but for
> debugging reasons (in wireshark) we included a return string that will be
added
> to the response body

Acknowledged.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/IndexController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/IndexController.java:18: class
IndexController
On 2015/10/07 15:00:27, robert.erneborg wrote:
> On 2015/10/07 09:26:10, kirill.sc wrote:
> > What does IndexController suppose to do? Single function here has nothing to
> do
> > with "Index(ing)"
> 
> regarding the name it is temporary and the reason behind it is that it
> corresponds to the single / url which in a regular http server returns
> index.html. However io the future I plan to reduce the number of controller
> classes to one single with all the methods from the current controllers.

Acknowledged.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/IndexController.java:40: return
"Server is upppppp";
On 2015/10/07 15:00:27, robert.erneborg wrote:
> On 2015/10/07 09:26:10, kirill.sc wrote:
> > Server probably is __not__ upppppp
> 
> the message queue was part of testing in an earlier stage and should be
removed.
> If server is up is returned it actually means that the http server is up. So
to
> also include the first comment here we wanted that the single / url would tell
> us information about the server status for debugging. We will probably add
more
> statuses here.

Acknowledged.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/UpdateController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/UpdateController.java:35: public
String updateRequest(@RequestBody GeoEntry entry) throws
IllegalArgumentException, IllegalAccessException {
On 2015/10/07 15:00:27, robert.erneborg wrote:
> On 2015/10/07 09:26:10, kirill.sc wrote:
> > I think here you can/should just reuse writeRequest
> 
> yo mean that the client instead should use write when (if) performing update?

I think this is one of the ways. We don't ask to do update operations in this
project. Technically it is the same write which can fail if the key doesnt
exist. I leave it up to you.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/WriteController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/WriteController.java:42: if
(entry.getValues().size() != 1){
On 2015/10/07 15:00:27, robert.erneborg wrote:
> On 2015/10/07 09:26:10, kirill.sc wrote:
> > isEmpty()?
> > or
> > This table __should__ have one field. ?
> 
> We have since this commit changed the structure of GeoEntry and we're not
using
> hashmap any more! So this code has been modified and corrected

Acknowledged.

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/WriteController.java:59: return
"Value added for key: " + entry.getKey();
On 2015/10/07 15:00:27, robert.erneborg wrote:
> On 2015/10/07 09:26:10, kirill.sc wrote:
> > Again, why such return, is it some YCSB requirement?
> 
> We want the client to know that the message was received and the parameters
sent
> was ok. do you think we shouln't let the client know if something was wrong
with
> the request?

I was confused about this return. Where do you generate actual http ok?

[robert.erneborg, 2015-10-07 18:21:32]

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/WriteController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/WriteController.java:50: value =
entry.getValues().get(c);
On 2015/10/07 09:26:10, kirill.sc wrote:
> What is the purpose of this loop? Are you trying to get the last key in a
> hashmap? (I dont think it is actually sorted) (you can do this without a loop)
> Also this goes against values.put("field0", value); in your read function.

not using this any more

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/WriteController.java:59: return
"Value added for key: " + entry.getKey();
On 2015/10/07 17:18:14, kirill.sc wrote:
> On 2015/10/07 15:00:27, robert.erneborg wrote:
> > On 2015/10/07 09:26:10, kirill.sc wrote:
> > > Again, why such return, is it some YCSB requirement?
> > 
> > We want the client to know that the message was received and the parameters
> sent
> > was ok. do you think we shouln't let the client know if something was wrong
> with
> > the request?
> 
> I was confused about this return. Where do you generate actual http ok?

At the top we annotate @ResponseStatus(HttpStatus.OK) which means that if the
method succeeds http ok will be sent along with a potential return value (Spring
will generate the actual response). If exception is thrown it will send an http
error (dont remeber which one, 404?). In ControllerAdviceHandler.java we can
catch specific exceptions and specify what error code we want to send etc.

[kirill.sc, 2015-10-07 21:59:54]

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
File src/main/java/se/kth/crypt/server/controller/WriteController.java (right):

https://codereview.appspot.com/268270043/diff/1/src/main/java/se/kth/crypt/se...
src/main/java/se/kth/crypt/server/controller/WriteController.java:59: return
"Value added for key: " + entry.getKey();
On 2015/10/07 18:21:32, robert.erneborg wrote:
> On 2015/10/07 17:18:14, kirill.sc wrote:
> > On 2015/10/07 15:00:27, robert.erneborg wrote:
> > > On 2015/10/07 09:26:10, kirill.sc wrote:
> > > > Again, why such return, is it some YCSB requirement?
> > > 
> > > We want the client to know that the message was received and the
parameters
> > sent
> > > was ok. do you think we shouln't let the client know if something was
wrong
> > with
> > > the request?
> > 
> > I was confused about this return. Where do you generate actual http ok?
> 
> At the top we annotate @ResponseStatus(HttpStatus.OK) which means that if the
> method succeeds http ok will be sent along with a potential return value
(Spring
> will generate the actual response). If exception is thrown it will send an
http
> error (dont remeber which one, 404?). In ControllerAdviceHandler.java we can
> catch specific exceptions and specify what error code we want to send etc.

ok,I see, cool.

[kirill.sc, 2015-11-21 08:57:54]

Removing myself from reviewers, issue can be closed