Sam Ottenhoff

This looks good to me Matt. This seems sane and like it should be enabled by default.

This looks good to me Matt. This seems sane and like it should be enabled by default.

Looks okay to me.

Looks okay to me.

This looks like it is from another patch.

This looks like it is from another patch.

This looks like it is from another patch.

This looks like it is from another patch.

I have reviewed this patch. It looks good to me.

I have reviewed this patch. It looks good to me.

SAM-2324 Range requests in ShowMediaServlet
SAM-2324 Range requests in ShowMediaServlet
Hi Jaques, The way Sakai patches are usually done is that one discreet issue is addressed with a specific patch and one commit to the Subversion repository. This patch represents many separate issu...

Hi Jaques, The way Sakai patches are usually done is that one discreet issue is addressed with a specific patch and one commit to the Subversion repository. This patch represents many separate issues from EMIs to general code cleanup.

How did you generate this patch? Do you have the possibility of generating distinct patches from separate commits to a source control repository? Ideally, this patch would become several JIRAs: one for EMIs, one for comments, one for generics, one for general cleanup, etc.

--Sam

Is there a way to separate out your general improvements to Samigo from the minimum functionality needed to implement the extending matching questions?

Is there a way to separate out your general improvements to Samigo from the minimum functionality needed to implement the extending matching questions?

This is a pretty massive patch with lots of whitespace changes, comment changes, import changes, etc., that are most likely not essential for this functionality. I don't see any way we can consider...

This is a pretty massive patch with lots of whitespace changes, comment changes, import changes, etc., that are most likely not essential for this functionality. I don't see any way we can consider this patch for Sakai 10 unless this patch is made substantially smaller.

Please avoid whitespace changes or formatting changes to existing code. It makes code history, merges, reviews, etc more difficult.

Please avoid whitespace changes or formatting changes to existing code. It makes code history, merges, reviews, etc more difficult.

This works makes total sense to me and is implemented exactly how I imagined... nice

This works makes total sense to me and is implemented exactly how I imagined... nice

I can say generally that we do hear this request from clients: some do want an explicit way to know when a user was added/dropped.

I can say generally that we do hear this request from clients: some do want an explicit way to know when a user was added/dropped.

Can ROLE_NAME refer to SAKI_REALM_ROLE instead?

Can ROLE_NAME refer to SAKI_REALM_ROLE instead?

I think USER_ID should be either 99 or 36 chars

I think USER_ID should be either 99 or 36 chars

This should be M_log

This should be M_log

I added one comment about the fileupload additions, but rest of code looks good to me.

I added one comment about the fileupload additions, but rest of code looks good to me.

What is the uploadMaxFileSize for?

What is the uploadMaxFileSize for?

Looks good.... Only issue I see is inconsistent tabs/spaces

Looks good.... Only issue I see is inconsistent tabs/spaces

How about a Sakai event at the beginning and end of the import? This would allow us to track down problematic users/imports.

How about a Sakai event at the beginning and end of the import? This would allow us to track down problematic users/imports.

Can we name this thread so that we could potentially track down the problem if it kills the JVM? thread.setName(userId + "-" + siteId) ?

Can we name this thread so that we could potentially track down the problem if it kills the JVM?

thread.setName(userId + "-" + siteId) ?

Only compile the Patterns once

Only compile the Patterns once

I believe the Pattern should be initialized and compiled at startup time instead of being compiled for every check.

I believe the Pattern should be initialized and compiled at startup time instead of being compiled for every check.