First-cut Support for kXR_fattr

[cal-jlab]

Implemented request/response objects and handler methods.

[cal-jlab]

Thanks for the feedback, Al. Sorry about the messy code. I gave up ages ago trying to keep consistent formatting, though I appreciate its value. I'll figure out how to tidy the code in accordance with the preferred style.

I wasn't sure how best to allocate the ByteBuf. Would it be something like ByteBufAllocator.DEFAULT.directBuffer()?

[alrossi]

For the allocator: yes, you could do that. I wasn't worried so much about whether it was pooled or not, just heap vs direct. But for what you are doing I really don't think it matters much.

The way I work is to write the code the way I normally do, then just before posting run the reformatting on the added or changed parts. I don't know what you are using to write code but applying reformatting is usually pretty easy in IDEs like IDEA, Eclipse, Visual Studio Code, etc.

This is not to be authoritarian and anal, it is just so that when we do diffs on commits we don't see a lot of stylistic changes cluttering it up. This was actually a recent change in praxis (only about a year ago) that we required everyone to do this ...

Thanks again for the contribution.

[alrossi]

Oh, I missed something ... whoops.

Since the FattrResponse does not implement ReferenceCounted, the bb is not being deallocated. So that you don't leak memory, you need to do this. Look at the ReadResponse for an example (I would suggest implementing ReferenceCounted and overriding, doing something like this):

@Override
    public int refCnt() {
        return bb.refCnt();
    }

    @Override
    public ReadResponse retain() {
        bb.retain();
        return this;
    }

    @Override
    public ReadResponse retain(int increment) {
        bb.retain(increment);
        return this;
    }

    @Override
    public boolean release() {
        return bb.release();
    }

    @Override
    public boolean release(int decrement) {
        return bb.release(decrement);
    }

    @Override
    public ReferenceCounted touch() {
        bb.touch();
        return this;
    }

    @Override
    public ReferenceCounted touch(Object hint) {
        bb.touch(hint);
        return this;
    }

[cal-jlab]

I tend to avoid using direct buffers in cases where they will not ultimately be passed to I/O functions. Just saw your note about reference count / deallocation. Will fix. Also, I am using eclipse for my IDE, so I imagine I'll be able to find a template for the java style. I'll digest your recent comments and follow up when I've fully grokked it all.

[alrossi]

"I tend to avoid using direct buffers in cases where they will not ultimately be passed to I/O functions"

Agreed. Hence, not a critical (or even necessary) change.

[alrossi]

Thanks for the update. I've asked @kofemann to take a quick peek just to make sure I didn't miss anything. After that, I think we can merge.

[cal-jlab]

Note that I don't really have any sanity check on the requests, so a misbehaving client could set nattr != len(nvec), for example. But it wasn't clear to me how best to handle malformed requests, since none of the constructors throw exceptions.

[alrossi]

Good point. This may not have been the best design decision, but it seems that in general some RuntimeException is expected. For instance, if you look at

public AbstractXrootdRequest(ByteBuf buffer, int requestId) {
        this(buffer);
        checkState(this.requestId == requestId);
    }

that throws an IllegalArgumentException. In general, I guess the expectation is that the parent decoder class catches the exception and wraps it in DecoderException (also Runtime) (see ByteToMessageDecoder line 467), expecting it eventually to bottom out in pipeline.fireExceptionCaught(e); But the problem is that this also relies on the proper handling of exceptionCaught downstream. For most cases, I think this will be handled correctly, though I would not swear this would not end up looking like a bug (stack trace output).

For now, if you want to add sanity checks, you might want to follow suit using the Guava Preconditions utilities.

[cal-jlab]

Actually, the only sanity check that seemed necessary was the nattr field. I committed that change and am happy to call this "done for now", if you all are satisfied.

[alrossi]

I'm good with this. Let's give Tigran a day or so to have a look. If he is unable to I will just go ahead and merge this.

Thanks again Chris.

[cal-jlab]

And a "thank you" to you as well. I appreciate your taking the time to help me make this contribution. I may encounter a few more unfinished corners as I work on fleshing out the xrootd interface to our tape system. If I do, I hope you don't mind my popping back over here for a chat ;-)

[alrossi]

Not at all, I'd be happy to.

Cheers, Al

[lemora]

As to the reformatting, if it's not too much hassle to apply another code style, I'll leave the link to our adapted sheet here as the usage 4 in stead of 2 white spaces does make a large difference in how the code looks and so currently your contribution feels a bit inconsistent with the rest of the xrootd4j code base. Sorry to be nitpicky, I hope it's not too much of an effort.

[alrossi]

@lemora I believe Chris uses Eclipse, so he would need the Eclipse version (unless Eclipse automatically converts IntelliJ xml into its config format ... I haven't used Eclipse in almost a decade now, pardon my ignorance). On the other hand, maybe all he needs to do is change the 2 spaces to 4 (was that the only change? I don't recall ...)

[alrossi]

Oh, yikes, I just ran install and the formatting on the header is also wrong. Mysterious, because it looks the same. But I wouldn't bother messing with this. Before merging I can just correct it.

[alrossi]

One thought, then, in connection with the formatting.

For future contributions it would be good if Chris could set up his Eclipse to use our style. But why don't I just reformat the two new files before committing, since I may need to tweak the header? That may be easiest at this point. (Probably best to manually merge squash in my local repo and then push the changes, rather than using GitHub.)

[alrossi]

I pushed the formatting changes to cal-jlab/xrootd4j.

[lemora]

@alrossi Changing two white spaces to four was the only change, other than agreeing on allowing import ordering but no automated rearranging or cleaning of code (which is irrelevant for new code, I guess).

[cal-jlab]

Sorry, I had just loaded up the style template and applied it to the two new files. Never looked to see if it was consistent. Just assumed tab spacing was an attribute of the template. I'll make sure to set it to four.

[cal-jlab]

I'm not sure what happened with the headers on the two files. Very strange. @alrossi are there any more changes I need to make, or were you going to do them in the merge? If I make changes, it could end up messing with the formatting again; I haven't really time at present to delve the subtle depths of the style guide (though I admit it's something I should do).

[alrossi]

Chris, as I said I think it best for me to worry about the formatting/headers at present. (Turns out there were two extra wsp the header verification did not like).

If you made the one change to Arrays. requested by Tigran, I think your job is done here!

Al

[cal-jlab]

Do you want to just make the Arrays.toString() change? I don't know how the headers got messed up, and assume they will again. My plan was to look into using IntelliJ for future contributions.

[alrossi]

Sure, I'll take care of it, NP.

[alrossi]

Merging manually.

[alrossi]

Committed: master@0435c00e4ae08b993d85a6879e28a913a05696e4